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

Fixes #37560 - rewrite React GH action #11053

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

jeremylenz
Copy link
Member

@jeremylenz jeremylenz commented Jun 26, 2024

What are the changes introduced in this pull request?

Change the 'React Tests' GitHub action to use @theforeman/actions in line with Foreman CI.

This will also allow us to use foreman_version to test Katello PRs against any Foreman PR or branch.

Considerations taken when implementing this change?

Also removed '(Non-blocking)' from the name, so it's assumed this is a blocking check. But I think we need a GH org admin to make that actually true.

What are the testing steps for this pull request?

critique my beautiful YAML
verify that CI passes and that the output reflects actual test runs

@jeremylenz jeremylenz force-pushed the 37560-react-tests-gh branch 3 times, most recently from 9d66994 to d9cd342 Compare June 26, 2024 21:21
@jeremylenz jeremylenz marked this pull request as ready for review June 27, 2024 14:34
@jeremylenz jeremylenz force-pushed the 37560-react-tests-gh branch 2 times, most recently from a3ce51d to bda2222 Compare June 27, 2024 15:08
.github/matrix.json Outdated Show resolved Hide resolved
@jeremylenz
Copy link
Member Author

build Expected — Waiting for status to be reported

I'm not sure what's wrong here.

@ekohl
Copy link
Member

ekohl commented Jun 27, 2024

I'm not sure what's wrong here.

There's probably a required check configured for this branch that now no longer exists. Some admin needs to fix that.

@@ -1,45 +1,23 @@
name: React Tests (non-blocking)
Copy link
Member

Choose a reason for hiding this comment

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

This, albeit the name, used to be a required check in GH.
After the rename, it's not matching anymore, so you get the "waiting" job.

I don't think this needs to be a "required check", but if you want it to be, it needs to run on every PR, not limited to paths like below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh, I thought it was my rename of build to react-tests. That explains why changing it back to build didn't work.

@parthaa I would like to call this "React Tests" (without the "(non-blocking)"). Can you please make "React Tests" the required check and remove "React Tests (non-blocking)" ?

Copy link
Member

Choose a reason for hiding this comment

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

@jeremylenz you probably need to merge this first before you can add it as a required check.

@jeremylenz
Copy link
Member Author

I had Partha remove the build required check and add a new required check for react-tests. I renamed the job here to react-tests and also removed the paths so it now runs on all pull requests. Let's see how it goes..

@jeremylenz
Copy link
Member Author

Looks like we might still have to merge this before the expected check is satisfied..

@jeremylenz jeremylenz requested a review from parthaa June 28, 2024 15:28
Copy link
Contributor

@parthaa parthaa left a comment

Choose a reason for hiding this comment

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

ack

@ekohl
Copy link
Member

ekohl commented Jun 28, 2024

Looks like we might still have to merge this before the expected check is satisfied..

That's also my experience

@parthaa parthaa merged commit 9dbfc29 into Katello:master Jun 28, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants