Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

electrocucaracha
Copy link
Member

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 the make fmt instruction which fixes the script and YAML formats.

@arora-sagar
Copy link
Contributor

@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.

@electrocucaracha
Copy link
Member Author

@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

@arora-sagar
Copy link
Contributor

arora-sagar commented Mar 26, 2024

@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.

@electrocucaracha
Copy link
Member Author

@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.

Copy link
Contributor

@efiacor efiacor left a 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)" \
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

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)"
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

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

@vjayaramrh
Copy link
Contributor

@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.

+1

@electrocucaracha
Copy link
Member Author

@tliron @henderiw I think that this is ready to be merged

@efiacor
Copy link
Contributor

efiacor commented May 15, 2024

/approve
/lgtm

Copy link
Contributor

nephio-prow bot commented May 15, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: efiacor, electrocucaracha
Once this PR has been reviewed and has the lgtm label, please assign tliron for approval by writing /assign @tliron in a comment. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants