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

fix: use argocli image from PR in CI + revert #12736 #13018

Merged
merged 2 commits into from
May 8, 2024

Conversation

Joibel
Copy link
Member

@Joibel Joibel commented May 7, 2024

Fix for CI

This PR fixes CI. It is in two commits, and best read as such

  1. Pin the images used to the ones from the PR instead of the ones from quay.io.
  2. Revert feat: add sqlite-based memory store for live workflows. Fixes #12025 #12736

The argocli image is now built and injected into k3d in CI. All uses of it and argoexec in CI are modified to have imagePullPolicy: Never to fail if they're not using it. (This is not ideal, we should use a different image and or tag to avoid accidents in future). All uses are now prefixed with quay.io as that's the name we generate.

The revert of #12736 fixes CI, without the revert it still fails (locally). Even with the revert but without building argocli and pushing it into k3d it will fail. Once argocli is built and pushed test-executor passes.

@Joibel Joibel marked this pull request as draft May 7, 2024 22:13
@Joibel Joibel changed the title Argocli image fix: argocli image used in CI from PR May 7, 2024
@Joibel Joibel changed the title fix: argocli image used in CI from PR fix: use argocli image from pull request in CI May 8, 2024
@Joibel Joibel marked this pull request as ready for review May 8, 2024 07:26
@Joibel Joibel added area/build Build or GithubAction/CI issues P1 High priority. All bugs with >=5 thumbs up that aren’t P0, plus: Any other bugs deemed high priority prioritized-review For members of the Sustainability Effort labels May 8, 2024
Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

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

LGTM

@isubasinghe isubasinghe merged commit 2f15709 into argoproj:main May 8, 2024
30 of 31 checks passed
@isubasinghe
Copy link
Member

isubasinghe commented May 8, 2024

When I looked into this issue, the CLI was calling ListWorkflows, however ListWorkflows was returning an empty list.
It makes sense to revert #12736 in this context.

@agilgur5
Copy link
Contributor

agilgur5 commented May 8, 2024

When I looked into this issue, the CLI was calling ListWorkflows, however ListWorkflows was returning an empty list.

That may add up to my comment in #12736 (comment)

Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

I think the CI changes can be optimized.

Also, I would've personally taken a more targeted approach to fixing this than a full revert (which is going to make the history a lot messier on top of other things); there's only a handful of lines from #12736 that affect non-Server CLI commands

.github/workflows/ci-build.yaml Show resolved Hide resolved
.github/workflows/ci-build.yaml Show resolved Hide resolved
if err != nil {
return nil, nil, err
}
return ctx, &argoKubeClient{instanceIDService, wfClient, wfStore}, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

per #12736 (comment), I suspect this is the erroring block

@agilgur5 agilgur5 changed the title fix: use argocli image from pull request in CI fix: use argocli image from pull request in CI + revert #12736 May 8, 2024
@agilgur5 agilgur5 changed the title fix: use argocli image from pull request in CI + revert #12736 fix: use argocli image from PR in CI + revert #12736 May 8, 2024
@agilgur5 agilgur5 added this to the v3.5.x patches milestone May 8, 2024
@Joibel Joibel deleted the argocli-image branch May 9, 2024 07:13
agilgur5 pushed a commit that referenced this pull request May 27, 2024
Signed-off-by: Alan Clucas <[email protected]>
Co-authored-by: Anton Gilgur <[email protected]>
(cherry picked from commit 2f15709)
@agilgur5
Copy link
Contributor

Backported to release-3.5 as c18b1d0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues P1 High priority. All bugs with >=5 thumbs up that aren’t P0, plus: Any other bugs deemed high priority prioritized-review For members of the Sustainability Effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants