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: don't rebuild ui/dist/app/index.html in argocli-build stage #13023

Merged
merged 3 commits into from
May 8, 2024

Conversation

Joibel
Copy link
Member

@Joibel Joibel commented May 8, 2024

The timestamps in CI are incorrect, and may result in an attempt to rebuild this file when we don't have yarn installed. So don't.

This is a problem introduced in #13018. edit by agilgur5: See also Slack thread for some debugging details on this

The timestamps don't always line up like expected when we come to check if we need to rebuild ui/dist/app/index.heml in the argocli-build step so force them fixed.

image

The timestamps in CI are incorrect, and may result in an attempt to
rebuild this file when we don't have yarn installed. So don't.

Signed-off-by: Alan Clucas <alan@clucas.org>
@Joibel Joibel added the area/build Build or GithubAction/CI issues label May 8, 2024
@Joibel Joibel added the prioritized-review For members of the Sustainability Effort label May 8, 2024
@Joibel
Copy link
Member Author

Joibel commented May 8, 2024

This should have run e2e tests really, but it didn't.

Dockerfile Outdated Show resolved Hide resolved
@agilgur5
Copy link
Contributor

agilgur5 commented May 8, 2024

This should have run e2e tests really, but it didn't.

We may want to add Dockerfile to common in the changed-files list

@agilgur5
Copy link
Contributor

agilgur5 commented May 8, 2024

This is a problem introduced in #13018.

See also Slack thread for some debugging details on this

@Joibel
Copy link
Member Author

Joibel commented May 8, 2024

This should have run e2e tests really, but it didn't.

We may want to add Dockerfile to common in the changed-files list

See #13024

Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Alan Clucas <alan@clucas.org>
@agilgur5
Copy link
Contributor

agilgur5 commented May 8, 2024

Would like to rebase this on top of #13024 to make sure E2Es run and pass here, otherwise LGTM

@agilgur5 agilgur5 changed the title fix: don't rebuild ui/dist/app/index.html fix: don't rebuild ui/dist/app/index.html May 8, 2024
@agilgur5 agilgur5 changed the title fix: don't rebuild ui/dist/app/index.html fix: don't rebuild ui/dist/app/index.html in argo-cli stage May 8, 2024
@agilgur5 agilgur5 changed the title fix: don't rebuild ui/dist/app/index.html in argo-cli stage fix: don't rebuild ui/dist/app/index.html in argocli-build stage May 8, 2024
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.

thanks for debugging and fixing this Alan!

@agilgur5 agilgur5 enabled auto-merge (squash) May 8, 2024 20:26
@agilgur5 agilgur5 merged commit e3b0bb6 into argoproj:main May 8, 2024
29 checks passed
@Joibel
Copy link
Member Author

Joibel commented May 9, 2024

Thank you for testing and getting them through Anton.

@Joibel Joibel deleted the fix-argocli-build branch May 9, 2024 07:12
@agilgur5 agilgur5 added this to the v3.5.x patches milestone May 27, 2024
agilgur5 pushed a commit that referenced this pull request May 27, 2024
…13023)

Signed-off-by: Alan Clucas <alan@clucas.org>
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
(cherry picked from commit e3b0bb6)
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 prioritized-review For members of the Sustainability Effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants