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

infra: ensure template alignment across branches #5582

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

KKoukiou
Copy link
Contributor

This commit implements a vital check to ensure consistent template alignment between the master branch and other branches. It enforces that template files on the master branch remain the definitive source of truth.
This decreases maintainance effort by having less files.

@github-actions github-actions bot added infrastructure Changes affecting mostly infrastructure f41 labels Apr 18, 2024
@KKoukiou
Copy link
Contributor Author

This will run only on branches != master.

Success example: KKoukiou#4
Failure example: KKoukiou#3

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. However, this will start failing on PRs which are not related to the templates files. That could be pretty confusing because all the failures on PR should be related to PR.

I like this idea, but I would change to run this once a day from master branch instead of on PRs.

.github/workflows/template-check.yml Outdated Show resolved Hide resolved
.github/workflows/template-check.yml Outdated Show resolved Hide resolved
@KKoukiou
Copy link
Contributor Author

Looks good to me. However, this will start failing on PRs which are not related to the templates files. That could be pretty confusing because all the failures on PR should be related to PR.

I like this idea, but I would change to run this once a day from master branch instead of on PRs.

Moving that to run on cron schedule from master, will not prevent us to merge template inconsistencies on branches. I think it's ok to block merging PRs on a repository that's not 'correct'. Syncing some template changes should be after all not be that hard.

@jkonecny12
Copy link
Member

However, you will also put this "failed" burden on external contributors or people who are solving blocker bugs. I have a bad feeling that output of this would be that we will start merging Red PRs because "it's not caused by this PR".

@KKoukiou KKoukiou force-pushed the infra-templates-master branch 3 times, most recently from c8a9d87 to d8d8019 Compare April 18, 2024 12:23
Copy link
Contributor Author

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkonecny12 ok now it checks only affected template files from a PR.

This commit implements a vital check to ensure consistent template alignment between the master
branch and other branches. It enforces that template files on the master branch remain the
definitive source of truth.
This decreases maintainance effort by having less files.

This check will get only activated on PRs that touch template files,
in order to not block unrelated work on the branches.
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only issue I'm seeing now is that it will print you all the files not only changed ones. However, I think that is fine to go with for the sake of simplicity.

@jkonecny12
Copy link
Member

Thanks for another improvement!

@KKoukiou
Copy link
Contributor Author

/kickstart-test --waive infra only

@KKoukiou KKoukiou merged commit 8a278bd into rhinstaller:master Apr 19, 2024
13 checks passed
@KKoukiou KKoukiou deleted the infra-templates-master branch April 19, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f41 infrastructure Changes affecting mostly infrastructure
2 participants