-
Notifications
You must be signed in to change notification settings - Fork 24
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
Enable scripts to update and format packages #34
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Victor Morales <[email protected]>
Signed-off-by: Victor Morales <[email protected]>
Signed-off-by: Victor Morales <[email protected]>
@electrocucaracha Is there a way we can get this branch tested again test-infra? Probably for this we need to make another test-infra branch which pulls from here. That will be good just to verify that nothing has been affected. |
There is a way but I think that it requires some manual adjustments, let me try to document the steps to do it |
Thank you. It is not necessary to test it but more of a precaution and it will be easier to review. |
Well, eventually it's something that we'll have to implement for other catalog changes as well. |
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.
Few minor comments.
Agree with Sagar in that we prob need to run the changes through a pipeline somehow.
|
||
.PHONY: fmt | ||
fmt: | ||
sudo -E $(DOCKER_CMD) run --rm -u "$$(id -u):$$(id -g)" \ |
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.
Do we need to run as sudo?
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.
Well, this depends how docker/podman was configured on the develeper's machine
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.
Not ideal but if necessary I guess can't be avoided.
} | ||
|
||
# cert-manager | ||
CERT_MANAGER_VERSION="v$(get_github_latest_release cert-manager/cert-manager)" |
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.
Would it not be better/safer to pin the relevant versions?
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.
The idea of this script is run it once in a while to update the different packages, we can create a scheduled job in GH to autocreate PRs
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.
Do we need a Makefile?
Could we just run the yamlfmt from the main update script?
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're right, Makefile can be optional, I just thought it could be more easy to have it
+1 |
22ed1f4
to
863b166
Compare
Signed-off-by: Victor Morales <[email protected]>
Signed-off-by: Victor Morales <[email protected]>
Signed-off-by: Victor Morales <[email protected]>
863b166
to
b5a8cde
Compare
/approve |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: efiacor, electrocucaracha The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR adds the
update_packages.sh
bash script for helping to update changes on cert-manager, cluster-api and multus packages as well as themake fmt
instruction which fixes the script and YAML formats.