-
Notifications
You must be signed in to change notification settings - Fork 44
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
Misc fix #936
Misc fix #936
Conversation
Roming22
commented
Feb 8, 2024
- Do not run test.sh in debug mode by default.
- Run test.sh in debug mode in the CI
- Auto-formatting of modified shell scripts (you can turn off whitespace diff in the files view to make the review easier)
- Fix an issue that would make any change to the tekton-chains-signing-secret require a manual action for the change to be deployed. It is possible that to deploy this fix, the manual action of deleting the existing job might be required one last time.
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.
a question and very minor suggestion (which I won't argue strongly for if you don't like)
operator/gitops/argocd/pipeline-service/openshift-pipelines/chains-secrets-config.yaml
Outdated
Show resolved
Hide resolved
rh-pre-commit.version: 2.1.0 rh-pre-commit.check-secrets: ENABLED
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.
argocd comment helpful
See below - I'm still a little confused about how debug is turned on now, but maybe I am missing something
@@ -4,7 +4,6 @@ | |||
set -o errexit | |||
set -o nounset | |||
set -o pipefail | |||
set -x |
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.
OK so I see where you turned off debug by default, but I don't see where the --debug
arg is getting processed ... is that a default bash thing I'm missing ?
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 done in the parse_args function, around line 48/49.
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.
ah cool leveraging an existing arg whose impl was too far from the deltas of this PR for me to see ... thanks for the ptr
/test test-pipeline-service-upgrade-ocp-414 |
/test test-pipeline-service-deployment-ocp-414 |
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.
/lgtm