-
Notifications
You must be signed in to change notification settings - Fork 129
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: add --skip-ca-certificates flag #617
Conversation
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.
diff
should have parity with sync for the most part, but lacks support for this flag:
14:24:24-0700 esenin $ /tmp/bdeck diff -s /tmp/empty.yaml
deleting ca-certificate b368ce04-cacd-4d0c-8811-9c75a5b7fab5
deleting ca-certificate a37fcb6d-a452-42c9-989f-cd5c49c0828f
Summary:
Created: 0
Updated: 0
Deleted: 2
14:24:27-0700 esenin $ /tmp/bdeck sync -s /tmp/empty.yaml --skip-ca-certificates
Summary:
Created: 0
Updated: 0
Deleted: 0
14:24:29-0700 esenin $ /tmp/bdeck diff -s /tmp/empty.yaml --skip-ca-certificates
Error: unknown flag: --skip-ca-certificates
Other than that and the test version question, looks good to go.
|
||
for _, tc := range tests { | ||
t.Run(tc.name, func(t *testing.T) { | ||
kong.RunWhenKong(t, ">=2.7.0") |
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.
Is there a reason that this is gated at 2.7? CA certificates were added much earlier and I'm not aware of any API difference that'd affect the way this flag operates since.
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.
I did it mostly to avoid having N tests for this, due to schema differences on various version. If you think that's needed/good to have, I'll split the tests per-version
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.
What are the differences? If the flag functions the same either way and it's just a matter of the input/output objects not being identical across all versions, fine to leave the gate in place as-is, just add a comment to the effect of "ca_certificates first appeared in 1.3, but we limit to 2.7+ here because the schema changed and the entities aren't the same across all versions, even though the skip functionality works the same."
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.
Exactly. Done.
Since CA certificates are 'global' entities in Kong, they cannot be managed on a per-workspace basis, making it hard to be handled declaratively with decK. This introduces a new --skip-ca-certificates to sync/dump/diff/reset to make sure CA certs are ignored when needed.
7199cff
to
716c18b
Compare
Ops, I forgot to git add it :\ did it now |
Codecov Report
@@ Coverage Diff @@
## main #617 +/- ##
==========================================
- Coverage 45.56% 45.49% -0.07%
==========================================
Files 74 74
Lines 8450 8462 +12
==========================================
Hits 3850 3850
- Misses 4241 4253 +12
Partials 359 359
Continue to review full report at Codecov.
|
9ffdb37
to
661d1e9
Compare
661d1e9
to
1680a1b
Compare
* feat: add --skip-ca-certificates flag Since CA certificates are 'global' entities in Kong, they cannot be managed on a per-workspace basis, making it hard to be handled declaratively with decK. This introduces a new --skip-ca-certificates to sync/dump/diff/reset to make sure CA certs are ignored when needed. * tests: add integration tests for --skip-ca-certificates
Since CA certificates are 'global' entities in Kong,
they cannot be managed on a per-workspace basis,
making it hard to be handled declaratively with decK.
This introduces a new
--skip-ca-certificates
tosync/dump/diff/reset to make sure CA certs are
ignored when needed.