-
Notifications
You must be signed in to change notification settings - Fork 15
feat: add support for install deps #867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
2 similar comments
|
/run pipeline |
|
/run pipeline |
ocofaigh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much code duplication - lets sync up to see how we can handle here. Especially since this code would even be duplicated across multiple repos too. Perhaps time to leverage https://github.com/terraform-ibm-modules/common-bash-library
ocofaigh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have scripts/install-deps.sh and modules/kube-audit/scripts/install-deps.sh? We should only have 1 script that should be used by all the modules in this repo. If needed update the script to support only installing certain binaries if required
modules/kube-audit/main.tf
Outdated
| count = var.install_dependencies ? 1 : 0 | ||
| # change trigger to run every time | ||
| triggers = { | ||
| build_number = timestamp() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this to trigger every time. It only need to trigger if the null resource has to run again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess thats not possible, if we set triggers to other null_resource blocks, the install script will run after there is a change in the other null_resource block and not before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should look into this. I don’t like the fact we will always install as part of every plan. Perhaps there is some pre condition or lifecycle feature we could leverage in terraform?
scripts/install-deps.sh
Outdated
| # Optional custom URL prefix for all binaries | ||
| CUSTOM_KUBECTL_URL="${CUSTOM_KUBECTL_URL:-}" | ||
| CUSTOM_JQ_URL="${CUSTOM_JQ_URL:-}" | ||
| CUSTOM_OC_URL="${CUSTOM_OC_URL:-}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not documented anywhere? We should probably list the environment variabl overrides in the variable descrption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also rename them to:
CUSTOM_KUBECTL_URL->KUBECTL_DOWNLOAD_URLCUSTOM_JQ_URL->JQ_DOWNLOAD_URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just thinking that there might be authentication required for someones custom URL, but I guess its on them to make sure that handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the documentation to the main README
scripts/install-deps.sh
Outdated
| # Install: kubectl | ||
| ####################################### | ||
|
|
||
| # renovate: datasource=github-releases depName=kubernetes/kubernetes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you will need to add a custom renovate rule in renovate.json for this to work (copy the one in common-dev-asstes)
|
@ocofaigh , |
ocofaigh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aashiq-J We should not have to duplicate the script in the submodule. Can't we find syntax to reference it from the same location? Worst case, we could use a symlink, but I'm hoping we can use ../.. to maybe find it?
|
@ocofaigh , |
|
/run pipeline |
Still facing issues with the kube-audit script which required token, will revert the change for now and need to debug that issue further. |
|
/run pipeline |
|
@Aashiq-J can you please create an issue to investigate with the goal of adding back the provider data lookup to generate the access token? |
|
/run pipeline |
2 similar comments
|
/run pipeline |
|
/run pipeline |
|
@Aashiq-J can you please leave comments on why tests are failing as they fail so we know what kind of issues are tests are hitting |
|
/run pipeline |
|
last test failed due to this: Not related to the code change. |
|
🎉 This PR is included in version 3.74.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |

Description
Add script to install binaries which are required by the scripts in the module.
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers