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

github: remove CentOS6 PR check #1778

Closed
wants to merge 1 commit into from
Closed

Conversation

sxa
Copy link
Member

@sxa sxa commented Dec 22, 2020

To avoid confusion and not put off potential contributors we should remove this check for now until #1745 is resolved.

Signed-off-by: Stewart X Addison [email protected]

Checklist
  • commit message has one of the standard prefixes
  • FAQ.md updated if appropriate
  • other documentation is changed or added (if applicable)
  • playbook changes run through VPC or QPC (if you have access)
  • inventory changes, ensure bastillion is updated accordingly

@sxa sxa added the bug label Dec 22, 2020
@sxa sxa added this to the December 2020 milestone Dec 22, 2020
@sxa sxa self-assigned this Dec 22, 2020
@sxa sxa requested a review from gdams December 22, 2020 19:19
@gdams
Copy link
Member

gdams commented Dec 22, 2020

My only concern with this is that it's what is used to update the docker images for the builds.

@karianna
Copy link
Contributor

wasn't there a proposed fix coming from Will?

Signed-off-by: Stewart X Addison <[email protected]>
@sxa
Copy link
Member Author

sxa commented Dec 22, 2020

My only concern with this is that it's what is used to update the docker images for the builds.

There looks to be actions in there associated with PRs and pushes - is the push executed when it's merged? [NOTE: Original PR had the wrong but comented out - have adjusted to leave it on push] Otherwise aren't we pushing and updating the image on every PR creation regardless of whether it's approved?

Either way, presumably at the moment it's unable to update so is fundamentally non-functional right now and only leading to confusion?

@sxa
Copy link
Member Author

sxa commented Dec 22, 2020

wasn't there a proposed fix coming from Will?

I believe you're referring to https://github.com/AdoptOpenJDK/openjdk-infrastructure/pull/1765/files which has already been merged and does not appear to have fixed the issue in the github checks.

@Willsparker are you still looking at a further fix?

@Willsparker
Copy link
Contributor

That only solves part of the problem. See #1745
The same sed change needs to be done on the SCL repo - but due to that being added in the playbook, it needs to be an ansible task too.

@karianna
Copy link
Contributor

@Willsparker Is that something which can be resolved soon or should we land this instead?

@Willsparker
Copy link
Contributor

Willsparker commented Dec 23, 2020

I'll have it done by today :-)

I have a solution - just testing it now

@Willsparker
Copy link
Contributor

As of #1780, the CentOS6 PR check works. (VPC also shows that the C6 playbook is working on a vagrant VM, too). Closing this as it's unnecessary now :-)

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.

4 participants