feat: Trigger warning for PRs likely requiring updates to management-plane-charts #2219
Conversation
kyma-bot
added
cla: yes
medmes
changed the title
kyma-bot
added
size/L
kyma-bot
added
size/M
kyma-bot
added
size/L
kyma-bot
added
size/M
This reverts commit 6d15458.
kyma-bot
added
size/L
There was a problem hiding this comment.
Overall looks good, just some general suggestions:
- I think I would split this into two workflows, one verifying that manifests are up to date, one checking if there are relevant changes
- regarding the config changes, I think it would be more robust if we work on the actual output based on the configuration
- so instead of checking if there are changes on the config files, I would suggest that we generate the KLM manifests from PR and main and compare if there are changes
- we already did this in an earlier implementation that can be used as a base again: chore: Verify diff in manifests #1358
- the previous point will cover the configuration part, but the ticket also says that the e2e test files are relevant. Here I don't think that we have output to work on, so an approach where we just look if there is a diff in the files should be sufficient. The GH
actionsandscriptspart should be the most relevant. Also, on top level thescripts/testspart should be included in the check. - I personally prefer consistency in approaches whenever possible. Here, we have multiple cases where we share info between different steps. First, we use the
outputsof the previous steps, later, we use env variables. I would suggest to consistently useoutputsin this case.
…trigger_warning_job_on_pr
…er_warning_job_on_pr
…trigger_warning_job_on_pr
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, labeled, unlabeled] |
There was a problem hiding this comment.
Labeled and unlabeled were needed on the previous approach but not here. Let's remove them. And since opened, synchronize and reopened are the default types, I think we should omit it entirely.
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
See above, I would remove the default types
| - name: Install Dependencies | ||
| run: | | ||
| sudo apt update | ||
| sudo apt install -y make |
There was a problem hiding this comment.
Are you sure we need to install it explicitly? https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md#installed-apt-packages
Also looks like we are not even using make in this action? Is it a leftover from testing?
| - name: Get list of changed files | ||
| id: changed-files | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const { data: files } = await github.rest.pulls.listFiles({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.payload.pull_request.number, | ||
| }); | ||
| const configFiles = files.filter(file => file.filename.startsWith('config/')); | ||
| core.setOutput('configFiles', configFiles.map(file => file.filename).join(',')); |
There was a problem hiding this comment.
This is still targeting the config/ folder. These are the sources for generating the manifest output via make dry-run-control-plane. As we already check the output in the other action, I don't think it makes sense to check the sources here. Eventually, we only care about the output, not the sources.
So this one should be replaced with the other files that we care about, but don't have an output. These are:
.github/actions(if we change an action, we should make sure it still works in the other repos where we import these).github/workflows(if we change a workflow, we should check whether we also need to change the same in management-plane-charts)./tests/scripts(those are used by the actions, so again we should check if it still works in the other repos)versions.yaml(we should check if our tooling version changes still work in the other repos)
The workflow then also needs to be renamed. Maybe something like "check-pipeline-changes`
There was a problem hiding this comment.
The rest of this workflow looks good. Maybe update the descriptions to be more general and not only about /config files
| - name: Add PR Comment if Manifests Are Outdated | ||
| if: steps.compare-manifests.outputs.outdated_manifests == 'true' | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.payload.pull_request.number, | ||
| body: "❌ **Detected diff in manifests!** Run 'make manifests' and commit changes." | ||
| }); | ||
| github.rest.issues.addLabels({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.payload.pull_request.number, | ||
| labels: ["outdated-manifests"] | ||
| }); |
There was a problem hiding this comment.
The check above (compare-manifests) doesn't determine if the manifests are "outdated". It determines if there is a diff between main and PR which may be an intended thing. Please adjust the descriptions and variable names accordingly.
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.payload.pull_request.number, | ||
| body: "❌ **Detected diff in manifests!** Run 'make manifests' and commit changes." |
There was a problem hiding this comment.
This is also not entirely true anymore. See my other comment for this block. I would propose that we ignore the make manifests case for now and add that with a separate PR.
Description
This PR introduce a Github wokflow for detecting changes on the config directory which needs to be reflected into management-plane-chart repository.
Changes proposed in this pull request:
The PR was tested against this Upsttream PR in order to bypass permission complexity:
#2223
For the PR merger
After the Pull request is merged. Please delete the trigger_warning_job_on_pr branch from the upstream.
Related issue(s)
#2089