-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
feat: Adding skipSchemaValidation flag (#20771) #20831
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides one tweak, lgtm! Can you merge master and re-run codegen?
Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com>
036c154
to
3805f94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test! One tweak
Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com>
Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com>
Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20831 +/- ##
==========================================
+ Coverage 54.97% 55.00% +0.03%
==========================================
Files 324 324
Lines 55417 55427 +10
==========================================
+ Hits 30464 30488 +24
+ Misses 22334 22318 -16
- Partials 2619 2621 +2 ☔ View full report in Codecov by Sentry. |
@crenshaw-dev Are there any more changes you are looking for, or its good to go from your perspective? I think all the issues flagged above have been addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall PR looks good to me!! Just left a quick question for understanding the extra change.
@crenshaw-dev any more concerns about the PR?
@@ -5,3 +5,4 @@ entries: | |||
urls: | |||
- http://127.0.0.1:9080/argo-e2e/testdata.git/helm-repo/helm-1.0.0.tgz | |||
version: 1.0.0 | |||
name: helm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QQ: Is the name field required here? I do not think this change is related to skipSchemaValidation
flag changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is. In helm 3.16, this property is now required (its not documented anywhere, but the test errors were saying it was a missing required attribute). Given that helm 3.16 is a requirement for this feature, it was added here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@ishitasequeira no concerns from me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good!! Thanks @dmosesson !!
* Adding skipSchemaValidation flag Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> * Adding specific test for skip schema validation Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> * Fix merge conflict Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> * Fixing index.yaml to support helm 3.16 Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> --------- Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> Co-authored-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> Signed-off-by: Adrian Aneci <aneci@adobe.com>
* Adding skipSchemaValidation flag Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> * Adding specific test for skip schema validation Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> * Fix merge conflict Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> * Fixing index.yaml to support helm 3.16 Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> --------- Signed-off-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com> Co-authored-by: Daniel Mosesson (soandos) <danielmosesson@gmail.com>
This is a fantastic change, but appears to only apply to single source applications. Is there a plan to incorporate this feature when using multiple sources? |
@jdvorak8660 this should work fine for multi-source apps. The |
Thanks for the response. I did not experience this when I attempted to use the option for multiple sources, but I'll go back and make sure I wasn't making some silly mistake. When it didn't work as expected I assumed it was not an option as it is also not referenced in the documentation for multiple sources. Any chance you have an example of a working multi source application with this option I could reference? Edit: @crenshaw-dev I may have found my issue. Can you confirm this change was only made in 2.14 (latest release candidate) and is not available in the latest stable release 2.13.3? |
Checklist:
Closes #20771
Tentatively also closes:
This allows argo to deploy helm charts that have JSON schema files for helm charts in locations that do not have internet access.
Some notes:
make codegen
and had issues, andmake codegen-local
ran with some slightly different versions I suspect.