-
Notifications
You must be signed in to change notification settings - Fork 30
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: Trigger warning for PRs likely requiring updates to management-plane-charts #2219
base: main
Are you sure you want to change the base?
Changes from all commits
e84d05f
38d368a
fe9a0bc
4bab3ad
974263a
c124fd3
ec56411
57b6fa3
fc47bf8
baa1e06
c25021a
6d15458
3d01f0c
1608059
9742436
d3c0d9e
e1b8293
45a38c8
e60f1d9
a836812
b0e45ae
a00d234
dd2146d
f77d9ce
6dc46d6
4adb7df
4120aac
033c5af
e19d8bc
d981bc5
acf9f86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
name: "Check Config Changes" | ||
|
||
on: | ||
pull_request: | ||
types: [opened, synchronize, reopened, labeled, unlabeled] | ||
|
||
jobs: | ||
check-configs-changes: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout code | ||
uses: actions/checkout@v4 | ||
with: | ||
fetch-depth: 2 | ||
|
||
- name: Install Dependencies | ||
run: | | ||
sudo apt update | ||
sudo apt install -y make | ||
Comment on lines
+16
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
- 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(',')); | ||
Comment on lines
+21
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still targeting the So this one should be replaced with the other files that we care about, but don't have an output. These are:
The workflow then also needs to be renamed. Maybe something like "check-pipeline-changes` There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rest of this workflow looks good. Maybe update the descriptions to be more general and not only about |
||
|
||
- name: Evaluate Config Changes | ||
id: eval-changes | ||
run: | | ||
echo "Changed config files:" | ||
echo "${{ steps.changed-files.outputs.configFiles }}" | tr ',' '\n' | ||
if [ "${{ steps.changed-files.outputs.configFiles }}" != "" ]; then | ||
echo "⚠️ Config directory changes detected!" | ||
echo "config_changed=true" >> $GITHUB_OUTPUT | ||
else | ||
echo "✅ No changes in config directory." | ||
echo "config_changed=false" >> $GITHUB_OUTPUT | ||
fi | ||
|
||
- name: Add Warning if Config Files Changed | ||
if: steps.eval-changes.outputs.config_changed == '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: "⚠️ **Config folder changes detected!** Please review if manifest updates are necessary." | ||
}); | ||
|
||
- name: Add PR Label for Config Changes | ||
if: steps.eval-changes.outputs.config_changed == 'true' | ||
uses: actions/github-script@v7 | ||
with: | ||
script: | | ||
github.rest.issues.addLabels({ | ||
owner: context.repo.owner, | ||
repo: context.repo.repo, | ||
issue_number: context.payload.pull_request.number, | ||
labels: ["configs-changed"] | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
name: "Check if Manifests are Up-to-Date" | ||
|
||
env: | ||
PR_CACHE_KEY: pr-manifests-${{ github.run_id }}-${{ github.run_attempt }} | ||
MAIN_CACHE_KEY: main-manifests-${{ github.run_id }}-${{ github.run_attempt }} | ||
|
||
on: | ||
pull_request: | ||
types: [opened, synchronize, reopened] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above, I would remove the default types |
||
|
||
jobs: | ||
create-pr-manifests: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout PR branch | ||
uses: actions/checkout@v4 | ||
|
||
- name: Run 'make manifests' on PR branch | ||
run: | | ||
make dry-run-control-plane | ||
mkdir -p ./cache/pr | ||
mv ./dry-run/manifests.yaml ./cache/pr/manifests.yaml | ||
|
||
- name: Save PR manifests in cache | ||
uses: actions/cache@v3 | ||
with: | ||
path: ./cache/pr/ | ||
key: ${{ env.PR_CACHE_KEY }} | ||
|
||
create-main-manifests: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout main branch | ||
uses: actions/checkout@v4 | ||
with: | ||
ref: main | ||
|
||
- name: Run 'make manifests' on main branch | ||
run: | | ||
make dry-run-control-plane | ||
mkdir -p ./cache/main | ||
mv ./dry-run/manifests.yaml ./cache/main/manifests.yaml | ||
|
||
- name: Save main manifests in cache | ||
uses: actions/cache@v3 | ||
with: | ||
path: ./cache/main/ | ||
key: ${{ env.MAIN_CACHE_KEY }} | ||
|
||
diff-manifests: | ||
needs: | ||
- create-pr-manifests | ||
- create-main-manifests | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Restore PR manifests from cache | ||
uses: actions/cache@v3 | ||
with: | ||
path: ./cache/pr/ | ||
key: ${{ env.PR_CACHE_KEY }} | ||
|
||
- name: Restore main manifests from cache | ||
uses: actions/cache@v3 | ||
with: | ||
path: ./cache/main/ | ||
key: ${{ env.MAIN_CACHE_KEY }} | ||
|
||
- name: Compare Manifests | ||
id: compare-manifests | ||
run: | | ||
set +e | ||
DIFF_OUTPUT=$(diff ./cache/pr/manifests.yaml ./cache/main/manifests.yaml) | ||
EXIT_CODE=$? | ||
if [[ $EXIT_CODE != 0 ]]; then | ||
echo "❌ Detected diff in manifests!" | ||
echo "$DIFF_OUTPUT" | ||
echo "outdated_manifests=true" >> $GITHUB_OUTPUT | ||
exit $EXIT_CODE | ||
fi | ||
echo "✅ No diff in manifests, all good." | ||
echo "outdated_manifests=false" >> $GITHUB_OUTPUT | ||
|
||
- 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." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also not entirely true anymore. See my other comment for this block. I would propose that we ignore the |
||
}); | ||
github.rest.issues.addLabels({ | ||
owner: context.repo.owner, | ||
repo: context.repo.repo, | ||
issue_number: context.payload.pull_request.number, | ||
labels: ["outdated-manifests"] | ||
}); | ||
Comment on lines
+83
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
- name: Remove 'outdated-manifests' Label if Fixed | ||
if: steps.compare-manifests.outputs.outdated_manifests == 'false' | ||
uses: actions/github-script@v7 | ||
with: | ||
script: | | ||
const labelName = 'outdated-manifests'; | ||
const { data: labels } = await github.rest.issues.listLabelsOnIssue({ | ||
owner: context.repo.owner, | ||
repo: context.repo.repo, | ||
issue_number: context.payload.pull_request.number, | ||
}); | ||
if (labels.some(label => label.name === labelName)) { | ||
console.log(`Label "${labelName}" found, removing it.`); | ||
await github.rest.issues.removeLabel({ | ||
owner: context.repo.owner, | ||
repo: context.repo.repo, | ||
issue_number: context.payload.pull_request.number, | ||
name: labelName, | ||
}); | ||
} else { | ||
console.log(`Label "${labelName}" not found, skipping removal.`); | ||
} | ||
|
||
- name: Fail if Manifests Are Outdated | ||
if: steps.compare-manifests.outputs.outdated_manifests == 'true' | ||
run: | | ||
echo "❌ Manifests are outdated! Run 'make manifests' and commit changes." | ||
exit 1 |
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.
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.