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

GHA: build RHEL and Amazon Linux #9266

Merged
merged 6 commits into from
Mar 2, 2022
Merged

GHA: build RHEL and Amazon Linux #9266

merged 6 commits into from
Mar 2, 2022

Conversation

Al2Klimov
Copy link
Member

No description provided.

@Al2Klimov Al2Klimov self-assigned this Feb 28, 2022
@cla-bot cla-bot bot added the cla/signed label Feb 28, 2022
@Al2Klimov Al2Klimov force-pushed the feature/subscription branch 3 times, most recently from de3d77c to e388bc4 Compare February 28, 2022 15:33
@Al2Klimov Al2Klimov changed the title WIP GHA: build RHEL and Amazon Linux Feb 28, 2022
@Al2Klimov Al2Klimov removed their assignment Feb 28, 2022
@Al2Klimov Al2Klimov marked this pull request as ready for review February 28, 2022 16:30
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Can you try if it's possible to keep the old job names by using the matrix variables in the name option? This suggests that this might be possible and hopefully this would let us avoid having to mess around with the branch protection rules too much (and thus avoid a mess in all older PRs).

Comment on lines 68 to 83
docker login registry.icinga.com -u build-docker/sles --password-stdin <<<"$GITLAB_REGISTRY_RO_TOKEN"
docker login registry.icinga.com -u github --password-stdin <<<"$GITLAB_RO_TOKEN"
Copy link
Contributor

Choose a reason for hiding this comment

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

https://docs.gitlab.com/ee/user/profile/personal_access_tokens.html says that personal access tokens are supposed to be used with the username (even though the username is ignored at the moment, but given that there's a hint about this, I think that this might be changed in the future).

.github/workflows/rpm.yml Outdated Show resolved Hide resolved
@Al2Klimov
Copy link
Member Author

"old job names" like the ones below next to "Expected — Waiting for status to be reported"? Likely possible. But I consider them not worth it and the mess reasonable. Just read the summary ignoring the expected checks: "22 successful and 6 expected checks" Also I don’t expect more of such refactories soon and we shall change the rules for each new distro anyway despite old PRs.

So I opt for letting them as is. Even if we shall set the names explicitly, they shall be actual names, not the current ones.

@julianbrost
Copy link
Contributor

"old job names" like the ones below next to "Expected — Waiting for status to be reported"?

Yes.

Also I don’t expect more of such refactories soon

So even more reasons for having fixed names.

we shall change the rules for each new distro anyway despite old PRs.

That's something different. ".rpm (centos, 7)" and ".rpm (centos, 7, false)" is fundamentally the same check

So I opt for letting them as is. Even if we shall set the names explicitly, they shall be actual names, not the current ones.

I'd also be okay with making this the PR changing it to "actual names", though I think the previous names are fine. The new names just add a useless true/false without context that's just an implementation detail (.deb actually suffers from that same problem already).

@Al2Klimov
Copy link
Member Author

Also I don’t expect more of such refactories soon

So even more reasons for having fixed names.

Errm... no? Actually it's the opposite.

we shall change the rules for each new distro anyway despite old PRs.

That's something different.

Yes... ? And... what's the point? Old PRs should be rebased anyway for the sake of the actually added checks. Same for future refactories we don’t know of, yet.

.deb actually suffers from that same problem already

It... suffers?

@julianbrost
Copy link
Contributor

Also I don’t expect more of such refactories soon

So even more reasons for having fixed names.

Errm... no? Actually it's the opposite.

Sorry, ignore that part, I somehow missed the important word "don't" when reading this.

we shall change the rules for each new distro anyway despite old PRs.

That's something different.

Yes... ? And... what's the point? Old PRs should be rebased anyway for the sake of the actually added checks. Same for future refactories we don’t know of, yet.

Well, "should" is the important word here, we don't always do. When the names stay the same, you get a good summary of what is missing, rather than some chaos.

.deb actually suffers from that same problem already

It... suffers?

I consider ".rpm (centos, 7)" a better name than ".rpm (centos, 7, false)", so this alone would be reason enough for me to change this.

@julianbrost julianbrost merged commit ab70178 into master Mar 2, 2022
@icinga-probot icinga-probot bot deleted the feature/subscription branch March 2, 2022 08:09
@julianbrost julianbrost added this to the 2.14.0 milestone Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants