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.

Sign up for GitHub

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

Jump to bottom

fix: allow workflow.failures variable in exit hooks. Fixes #12789 #13492

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: allow workflow.failures variable in exit hooks. Fixes #12789 #13492

wants to merge 5 commits into from

Conversation

Bo0km4n
Copy link

@Bo0km4n Bo0km4n commented Aug 22, 2024 •
edited by agilgur5
Loading

Updated version of Original PR #12798

@Bo0km4n Bo0km4n mentioned this pull request Aug 22, 2024
Open
agilgur5
agilgur5 reviewed Aug 23, 2024
View reviewed changes
Copy link

@agilgur5 agilgur5 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It looks like you resolved #12798 (comment) but not #12798 (comment)

Also if you're directly quoting the previous PR in the description, please do use block quotes > for proper attribution. I added a few, but am on my phone so didn't get to all without a proper keyboard

@agilgur5 agilgur5 added area/hooks area/templating Templating with `{{...}}` labels Aug 23, 2024
@agilgur5 agilgur5 changed the title fix: allow workflow.failures variable in exit hooks. Fixes #12789 fix: allow workflow.failures variable in exit hooks. Fixes #12789 Aug 23, 2024
@Bo0km4n
Copy link
Author

Bo0km4n commented Aug 26, 2024

@agilgur5
Thank your fix.

It looks like you resolved #12798 (comment) but not #12798 (comment)

The above discussion is not yet complete, and just fixing the tests is not sufficient, is it?

@Bo0km4n
Copy link
Author

Bo0km4n commented Aug 30, 2024

@agilgur5 @tczhao ping

@tczhao
Copy link
Member

tczhao commented Oct 3, 2024 •
edited by agilgur5
Loading

Hi @Bo0km4n
Sorry, couldn't reply soon enough.

  • please fix the lint issue (reasoning: Using require.NoError instead of assert.NoError will allow the tests to fail faster. assert.NoError allows the test to continue, only failing at the very end. require.NoError fails immediately)
  • Unless there are other challenge, the approach in fix: Support validating template of exit hook. Fixes #13245 #13246 does make more sense to me compared to the current approach

@agilgur5
Copy link

agilgur5 commented Oct 10, 2024 •
edited
Loading

It looks like you resolved #12798 (comment) but not #12798 (comment)

The above discussion is not yet complete, and just fixing the tests is not sufficient, is it?

You'd have to resolve the comments in that discussion as well.

I'll let Tianchu take lead on this as my hands are more than full 😅

agilgur5
agilgur5 reviewed Oct 10, 2024
View reviewed changes
workflow/validate/validate_test.go

var workflowTemplateFailuresExitHook = `
apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
Copy link

Choose a reason for hiding this comment

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

I think you misunderstood #12798 (comment). It needs a test against a template level hook, not an equivalent WorkflowTemplate

@agilgur5 agilgur5 added the problem/more information needed Not enough information has been provide to diagnose this issue. label Oct 10, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity and needs further changes. It will be closed if no further activity occurs.

@github-actions github-actions bot added problem/stale This has not had a response in some time and removed problem/stale This has not had a response in some time problem/more information needed Not enough information has been provide to diagnose this issue. labels Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agilgur5 agilgur5 agilgur5 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/hooks area/templating Templating with `{{...}}`
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Bo0km4n @tczhao @agilgur5 @eduardodbr