-
Notifications
You must be signed in to change notification settings - Fork 104
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 553 resource level annotations #644
Feat 553 resource level annotations #644
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.
I would like to know what this means in terms of backward compatibility
Arrays.asList(instance.labels()).stream().map(i -> new io.dekorate.kubernetes.config.Label(i.key(), i.value(), | ||
i.kinds())).collect(Collectors.toList()).toArray(new io.dekorate.kubernetes.config.Label[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.
Formatting looks messed up
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.
Weird as we now use a maven plugin for formatiing. Will look closer.
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.
Formatter is not used by default. I will address this in a separate issue.
@@ -25,9 +25,7 @@ public void testGeneratedCode() { | |||
.withName("myapp") | |||
.withVersion("1.0.0") | |||
.withExpose(true) | |||
.addNewLabel("key1", "val1") | |||
.addNewLabel("key1", "val1", new String[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.
Do we really need to do this? Can't the existing addNewLabel
continue to exist?
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.
No, it can't.
Possible alternatives:
.addNewLabel().withKey("key1").withValue("val1").endLabel()
Given that this change is not user facing, I'd say its no biggie.
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.
Sure yeah, it just looks ugly
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.
@geoand: The main thing with these methods is that they are generated.
The logic is the following:
if the object is flat and has less than five 5 fields generate a with method with all fields
.
In theory we could create a method for each possible field combination. In the current case it would look like:
withNewLabel(String key);
withNewLabel(String key, String value);
withNewlabel(String key, String value, String[] kinds);
However, this would mean that we would also get methods that don't make sense.
Now, since to original object is an annotation and fields without default values are considered mandatory we could possibly work soemthing out.
I think that this would be one or two days work. and I'd like to avoid it at the moment.
Created: sundrio/sundrio#177
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.
@geoand if this sounds reasonable can we approve this?
There will be no backwards compatibility, but will be forward compatibility (partially). So, most probably this will require a 0.13.0 release. |
CI seems to be failing |
08afd82
to
74b374f
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Resolves: #553
This pull request introduces a new fields
kinds
on@Label
and@Annotation
. The use of these fields will limit the application of Labels/Annotations only to the listed kinds. If omitted, all kinds are asumed.