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(cli): argo lint with strict should report case-sensitive errors. Fixes #13006 #13250

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

miltalex
Copy link
Member

Fixes #13006

Motivation

Currently, when linting a CronWorkflow definition file, argo lint fails to detect keys with incorrect capitalization. This oversight can lead to unexpected behavior when applying these definitions using kubectl. Prior to this fix, argo cron create would ignore capitalization issues and proceed with the provided values, creating an inconsistency between kubectl and argo cron create behaviors.

By addressing this issue, we aim to:

  1. Enhance the linting process to catch capitalization errors early in the development cycle.
  2. Align the behavior of argo cron create/argo submit with kubectl, ensuring both tools report errors for incorrectly capitalized fields.
  3. Streamline the workflow creation process by providing consistent feedback across different Argo and Kubernetes tools.

This change will help prevent potential errors by catching and reporting capitalization issues uniformly across all relevant tools.

Modifications

To address this issue, we've made the following change:

Modified the strict path of the linting process to utilize the sigs.k8s.io/json library for unmarshaling.

  • This library provides case-sensitive unmarshaling in strict mode.
  • Fields with incorrect capitalization (e.g., "schedUle" instead of "schedule") will now be unmarshaled as-is.
  • The strict mode will then report these incorrectly capitalized fields as errors using the k8s.io/apimachinery/pkg/runtime library.

This change ensures that argo lint will now flag incorrectly capitalized fields when strict mode is enabled, providing developers with immediate feedback on potential issues in their workflow definitions. It also aligns the behavior of argo lint and argo cron create/argo submit with kubectl, creating a consistent experience across these tools.

Verification

To validate the effectiveness of this fix, I have extended the test suite with two new unit tests:

  1. A test case for strict mode enabled, verifying that capitalization errors are correctly reported.
  2. A test case for strict mode disabled, ensuring that the existing behavior remains unchanged.

Signed-off-by: Miltiadis Alexis <alexmiltiadis@gmail.com>
@miltalex miltalex force-pushed the fix/argo-lint-case-sensitive branch from e23a5ba to 8b4147d Compare June 26, 2024 10:55
Signed-off-by: Miltiadis Alexis <alexmiltiadis@gmail.com>
@miltalex miltalex marked this pull request as ready for review June 26, 2024 15:24
@Joibel Joibel self-assigned this Jul 9, 2024
Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

I am happy with this.

@agilgur5, I notice you were discussing making pkg do this strict check instead of doing it in here. That feels slightly better as an approach, but I don't want to block this on that. Would you rather we went that way?

@agilgur5 agilgur5 changed the title fix(cli): argo lint with strict should report case-sensitive errors. fixes #13006 fix(cli): argo lint with strict should report case-sensitive errors. Fixes #13006 Jul 11, 2024
@Joibel Joibel merged commit c834ef5 into argoproj:main Jul 18, 2024
29 checks passed
@miltalex miltalex deleted the fix/argo-lint-case-sensitive branch July 18, 2024 11:04
@agilgur5
Copy link
Contributor

agilgur5 commented Jul 19, 2024

@agilgur5, I notice you were discussing making pkg do this strict check instead of doing it in here. That feels slightly better as an approach, but I don't want to block this on that. Would you rather we went that way?

In #13006 (comment) I was primarily concerned about the scenario of "strict + case-sensitive". If the k8s variant handles this, then perhaps the functions in pkg are not even necessary and can be replaced and removed.

But if we wanted to, we could add this as an additional function in pkg and then load from the same place. For this case, I don't mind either way; would in fact prefer replacing and removing if possible.

@agilgur5 agilgur5 added this to the v3.5.x patches milestone Jul 30, 2024
agilgur5 pushed a commit that referenced this pull request Jul 30, 2024
…Fixes #13006 (#13250)

- `go.mod` merge conflict fixed by agilgur5

Signed-off-by: Miltiadis Alexis <alexmiltiadis@gmail.com>
Co-authored-by: Alan Clucas <alan@clucas.org>
(cherry picked from commit c834ef5)
Joibel added a commit to pipekit/argo-workflows that referenced this pull request Sep 19, 2024
…Fixes argoproj#13006 (argoproj#13250)

Signed-off-by: Miltiadis Alexis <alexmiltiadis@gmail.com>
Co-authored-by: Alan Clucas <alan@clucas.org>
Joibel added a commit that referenced this pull request Sep 20, 2024
…Fixes #13006 (#13250)

Signed-off-by: Miltiadis Alexis <alexmiltiadis@gmail.com>
Co-authored-by: Alan Clucas <alan@clucas.org>
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.

argo lint is not case sensitive
3 participants