refactor (jkube-kit/config/resource) : Deprecate jkube.io annotation prefix in favor of jkube.eclipse.org for JKubeAnnotations (#1273)
#1995
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Eclipse JKube CI ReportStarted new GH workflow run for #1995 (2023-02-23T17:28:57Z) ⚙️ JKube E2E Tests (4253717473)
|
rohanKanojia
force-pushed
the
pr/issue1273
branch
from
f7fa0a5
to
96b5817
Compare
Codecov Report
@@ Coverage Diff @@
## master #1995 +/- ##
============================================
- Coverage 55.65% 54.17% -1.49%
+ Complexity 4276 4058 -218
============================================
Files 481 480 -1
Lines 21190 21080 -110
Branches 2838 2815 -23
============================================
- Hits 11794 11420 -374
- Misses 8189 8491 +302
+ Partials 1207 1169 -38
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
rohanKanojia
force-pushed
the
pr/issue1273
branch
2 times, most recently
from
96b5817
to
f7d2045
Compare
manusa
reviewed
Jan 23, 2023
| @@ -617,6 +617,9 @@ For `Job`, this defaults to `OnFailure`. For others, it's not provided ({cluster | |||
|
|
|||
| | `serviceAccount` | |||
| | ServiceAccount name which will be used by pods created by controller resources(e.g. `Deployment`, `ReplicaSet` etc) | |||
|
|
|||
| | `useOldJKubePrefix` | |||
There was a problem hiding this comment.
nit: useLegacyJKubePrefix
I think that generally replacing old with legacy across this PR might give a better impression.
rohanKanojia
force-pushed
the
pr/issue1273
branch
from
f7d2045
to
16e9e3c
Compare
rohanKanojia
force-pushed
the
pr/issue1273
branch
from
16e9e3c
to
9936521
Compare
sunix
reviewed
Feb 15, 2023
| } | ||
|
|
||
| public String value() { | ||
| return annotation; | ||
| public String value(boolean useDeprecatedPrefix) { |
There was a problem hiding this comment.
To me it would be more elegant to set the useDeprecatedPrefix with a with method
JKubeAnnotations.GIT_COMMIT.withUseDeprecatedPrefix(true).value();
default with useDeprecatedPrefix to false:
JKubeAnnotations.GIT_COMMIT.value();
at the moment, when reading the code, it is not clear what is passed to value(arg)
There was a problem hiding this comment.
IMHO I'd simplify this, just provide two methods (no need for a public static method that is not reused anywhere?), a simple value with the boolean parameter that overloads and calls the value() method with false (default),
There was a problem hiding this comment.
I checked and getAnnotationPrefix method seems to be getting used in AutoTLSEnricher to determine jkube prefixes in mount points:
There was a problem hiding this comment.
I've created a separate issue to handle this case #2068 . I've moved this static method to a utility class
rohanKanojia
force-pushed
the
pr/issue1273
branch
3 times, most recently
from
8d90693
to
1d3b266
Compare
rohanKanojia
force-pushed
the
pr/issue1273
branch
2 times, most recently
from
ec9ac8b
to
62d4889
Compare
3 tasks
rohanKanojia
force-pushed
the
pr/issue1273
branch
2 times, most recently
from
ba1f5a8
to
f2f5f00
Compare
…n prefix in favor of `jkube.eclipse.org` for JKubeAnnotations (eclipse-jkube#1273) + Deprecate `jkube.io` annotation prefix in favor of `jkube.eclipse.org` + Add `jkube.useOldJKubePrefix` flag to switch to `jkube.io` annotation prefix in case any user is interested in this. Signed-off-by: Rohan Kumar <rohaan@redhat.com>
…prefix Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia
force-pushed
the
pr/issue1273
branch
from
f2f5f00
to
cd51945
Compare
manusa
reviewed
Feb 23, 2023
| @@ -38,7 +38,7 @@ endif::[] | |||
| | *tlsSecretVolumeMountPoint* | |||
| | Where the service serving secret should be mounted to in the pod. | |||
|
|
|||
| Defaults to `/var/run/secrets/jkube.io/tls-pem`. | |||
| Defaults to `/var/run/secrets/jkube.eclipse.org/tls-pem`. | |||
manusa
requested changes
Feb 23, 2023
…pse.org` + Mark `jkube.io/` annotations as Deprecated + Add `jkube.eclipse.org/` prefixed annotations to list of annotations + Add documentation for new ResourceConfig's field `useOldJKubePrefix` Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia
force-pushed
the
pr/issue1273
branch
from
cd51945
to
4ef486a
Compare
|
SonarCloud Quality Gate failed. |
manusa
approved these changes
Feb 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1273
jkube.ioannotation prefix in favor ofjkube.eclipse.orgjkube.useOldJKubePrefixflag to switch tojkube.ioannotation prefix in case any user is interested in this.jkube.eclipse.organnotation prefixSigned-off-by: Rohan Kumar rohaan@redhat.com
Type of change
test, version modification, documentation, etc.)
Checklist