-
Notifications
You must be signed in to change notification settings - Fork 38
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: introduce v1beta1 version #526
Conversation
Signed-off-by: odubajDT <[email protected]>
1bd9ac9
to
dcbcdf1
Compare
Signed-off-by: odubajDT <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #526 +/- ##
=======================================
Coverage 82.78% 82.78%
=======================================
Files 22 22
Lines 1423 1423
=======================================
Hits 1178 1178
Misses 207 207
Partials 38 38
Flags with carried forward coverage won't be shown. Click here to find out more. |
There is a problem with lint CI check, which is present on the main branch. This will be handled in a separate PR |
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Image string `json:"image"` | ||
|
||
// Tag to be appended to the sidecar image, defaults to 'main' | ||
// +optional | ||
Tag string `json:"tag"` |
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.
This is related to issue #517
This can be considered as a security concern and hence we can discuss on removing these
cc - @toddbaert @thisthat appreciate your opinion
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.
adapted the code to remove tag and image from FlagSourceConfiguration
CRD
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.
Ya I'm fine with removing them. I have only heard agreements on this.
I think we should come up with a short list of changes that we can include in the extended commit message.
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.
With further implementation I came across some issues regarding this. Read comment here #517 (comment)
I would suggest that covering this should be part of a separate PR, as it will bump the complexity of the PR, which will be adapting the code to use v1beta1
instead of v1alpha1
. Any objections on this ?
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.
|
||
const ( | ||
SyncProviderKubernetes SyncProviderType = "kubernetes" | ||
SyncProviderFilepath SyncProviderType = "filepath" |
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.
Please consider the diff highlighted in this related issue - #433
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.
Do we already have a unified solution? I see that the ticket does not clearly state which option to use
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.
@beeme1mr @Kavindu-Dodan @thisthat which is better here? Any opinions? I suppose since we're already doing breaking changes here, it makes sense to concentrate them all here... so perhaps we should conform here in OFO to the flagd style: file
.
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.
adapted to file
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.
fbcf24d
to
583669e
Compare
Signed-off-by: odubajDT <[email protected]>
Closing in favor of #535 |
This PR
Related Issues
Part-of #352
Part-of #433
Follow-up
Notes
There is a problem with lint CI check, which is present on the main branch. This will be handled in a separate PR