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

refactor: CI only pulls each minikube once for each kubernetes version #2691

Merged
merged 1 commit into from
Feb 22, 2025

Conversation

xstefank
Copy link
Collaborator

No description provided.

@@ -28,6 +28,8 @@ on:

jobs:
integration_tests:
name: "Integration tests (${{ inputs.kube-version }}, ${{ inputs.java-version }}, ${{ inputs.it-category }},
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is duplicated information which doesn't appear in the overview of the build outputs so we might as well remove it, imo

integration_tests:
name: "Integration tests (${{ inputs.kube-version }}, ${{ matrix.java }}, ${{ matrix.it-category }})"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, as we never call this workflow directly, the kube version is redundant and leaving it out would make the rest stand out more, imo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if we don't specify the name it will be only "integration_tests" so I opted to repeat the info. I'm not sure what would be better solution. Still remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I'm not sure if it's better like this, but at least the information is not repeated.

Verified

This commit was signed with the committer’s verified signature.
csviri Attila Mészáros
Signed-off-by: xstefank <xstefank122@gmail.com>
@xstefank
Copy link
Collaborator Author

I personally don't like it, because you don't see from the overview which job is which -
Untitled-2024-11-08-0753

@metacosm
Copy link
Collaborator

I personally don't like it, because you don't see from the overview which job is which - Untitled-2024-11-08-0753

Yeah, that's not great but it seems to be yet another level, not sure why it shows Experimental first either…

@xstefank
Copy link
Collaborator Author

@metacosm that's because we need to create matrix step by step and the each action/step has name. So before it was "integration_tests" a few times but we cannot skip the name in each step unless we would inline all actions into one file. Less readable but it would solve the naming issue and a few files that you need to go over when finding how it composes.

@csviri WDYT?

@csviri
Copy link
Collaborator

csviri commented Feb 20, 2025

do we really want to test all the clients? It should be the responsibility of fabric8. and there were long time no issues with that. (Alhough I know there are now issues popping up with vertex). Maybe having just that removed from the matrix would sole the issue we facing.

@csviri
Copy link
Collaborator

csviri commented Feb 20, 2025

I get that this works, but feels that it brings quite a complexity. Only issue what we are facing is throttling. Having just fewer overall categies solves the problem, even if the tests run a bit long. If we have to choose between running the tests bit longer on CI vs complexity in the CI. I would probably choose longer runs tbh.

@csviri
Copy link
Collaborator

csviri commented Feb 20, 2025

What I would propose is also take a look how this would look like, if we would just remove the testing of various client flavors, and remove the integration test categories as a separate PR and maybe compare with this approach. (Can try later just quite sick atm)

Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

Otherwise LGMT

@metacosm
Copy link
Collaborator

I would merge this and then figure out if we want to do something else. We need to address the quota issue first, imo.

@metacosm metacosm merged commit 0b47463 into operator-framework:main Feb 22, 2025
71 checks passed
@xstefank xstefank deleted the ci-refactor branch February 28, 2025 08:59
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.

None yet

3 participants