-
Notifications
You must be signed in to change notification settings - Fork 154
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
feat: validate kustomize build of tasks and pipelines #1792
Conversation
fdbe145
to
3033272
Compare
29e718a
to
fe32af3
Compare
fe32af3
to
c9884f3
Compare
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 task is mirrored from another repo, so changes made directly will eventually be overwritten by changes made there.
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.
Will ignore building manifest for this task then
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 task is mirrored from another repo, so changes made directly will eventually be overwritten by changes made there.
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.
same for this task 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.
This task is mirrored from another repo, so changes made directly will eventually be overwritten by changes made there.
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.
same for this task as well
55f6f38
to
04430c6
Compare
04430c6
to
fb458d7
Compare
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.
Could you please describe the solution in the commit message?
echo "Please run ./hack/build-manifests.sh and update your PR" >&2; | ||
exit 1; | ||
} | ||
fi |
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.
Looks like ./hack/build-manifests.sh
becomes a mandatory part of the development workflow when working with tasks and pipelines. Can you describe this? Probably inside the build-manifests.sh
script itself at least.
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.
Described it in the README change here. Do you think it is sufficient or need to be added to the build-manifests.sh
script 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.
The content linked tells what need to do for task and pipeline authors, it is not a description to the whole solution. In my mind, the description could tell, for example, what the task and pipeline authors should do, what the maintainers should do, how the kustomized manifests are validated, whether this validation changes the build-definitions task and pipelines contribution workflow, even the build-and-push, etc.
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.
Updated the README now. We don't need to change anything on the build-and-push
script.
fb458d7
to
7aaed3c
Compare
/retest |
1 similar comment
/retest |
# with the task name separated by a space, for example: | ||
# SKIP_TASKS="git-clone init" | ||
|
||
SKIP_TASKS="generate-odcs-compose provision-env-with-ephemeral-namespace verify-signed-rpms" |
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 allowing to skip tasks and pipelines?
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.
These tasks are mirrored from another repo, there is some automation which syncs them. If we change anything manually, that will be reverted. So skipped them.
echo "Please run ./hack/build-manifests.sh and update your PR" >&2; | ||
exit 1; | ||
} | ||
fi |
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 content linked tells what need to do for task and pipeline authors, it is not a description to the whole solution. In my mind, the description could tell, for example, what the task and pipeline authors should do, what the maintainers should do, how the kustomized manifests are validated, whether this validation changes the build-definitions task and pipelines contribution workflow, even the build-and-push, etc.
d3a6320
to
d03d2ea
Compare
d03d2ea
to
022edac
Compare
022edac
to
93d0038
Compare
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.
There is nothing left to review in this pull request from me as code owner of the sast-*
tasks.
93d0038
to
c8cb354
Compare
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, just minor details
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 change in sast-coverity-check
looks good.
de28fc0
to
e800ade
Compare
e800ade
to
f5ac2f6
Compare
After #1792 (comment), we don't need most of those reviews anymore 🙂 Just these |
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
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
This PR adds a CI check to validate kustomize build is successful and also checks that PR authors are committing built manifests files of task/pipeline change with the help of
hack/build-manifests.sh
script with their task/pipeline changes.Part of story: https://issues.redhat.com/browse/STONEBLD-3042