Describable will give a warning on multiple implementations of a symbol #674

dgarzon · 2018-12-06T21:49:46Z

This PR addresses #658 and #657 on the CasC side of things. Currently, if there are duplicate symbols both the configure and getImplementors methods throw ugly exceptions. Ideally, they should not throw and instead log a warning to the user to report the duplicates to maintainers.

I may have gone a bit overboard with the refactoring, but I think it is valuable nonetheless. The code now is easier to reason about as I have moved code to functions and given them clear names. Also tried to DRY this as much as possible.

Behavior:

getImplementors: if a duplicate key is found, we discard it, and log Found multiple implementations for symbol = fooBar: [foo.bar.FooBar, foo.baz.FooBar]. Please report to plugin maintainer.
configure: if a duplicate key is found, we log Found multiple implementations for symbol = fooBar: [foo.bar.FooBar, foo.baz.FooBar]. Please report to plugin maintainer. and configure the first instance returned by getDescriptors() which is the same behavior we had before this refactoring.

jetersen · 2018-12-07T07:56:50Z

Build failure: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fconfiguration-as-code-plugin/detail/PR-674/1/pipeline#log-245

timja

Looks reasonable, want to resolve the conflicts and try get a green build?

jetersen · 2019-03-19T20:56:40Z

💪 I have too much power @timja

dgarzon · 2019-03-19T21:00:11Z

Thank you @Casz and @timja I completely forgot about this PR :)

jetersen · 2019-03-19T21:00:43Z

@BlueAndi this should hopefully ~~fix~~ help you with duplicate (diagnosing the sinner) schema issue and together with #777 everything should be better.

Honestly me too @dgarzon 😆

jetersen · 2019-03-19T21:03:58Z

CI is having a field day 😢

jetersen · 2019-03-19T21:13:31Z

oh @dgarzon apparently it won't actually compile: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fconfiguration-as-code-plugin/detail/PR-674/23/pipeline#step-59-log-545

BlueAndi · 2019-03-19T21:14:55Z

@Casz Thanks for pointing this out. I hope I can check this tomorrow in the office.

jetersen · 2019-03-21T00:16:33Z

@dgarzon this breaks binary compatibility, however, since none of the custom plugin configurators have been merged I think we are safe.

It would require some rework in some of the PRs such as jenkinsci/job-dsl-plugin#1142

jetersen · 2019-03-21T01:21:23Z

@dgarzon and @timja please take a look 😄

jetersen · 2019-03-21T03:30:16Z

Moved my extra work to a separate PR, it started to become more work than I expected 😄

dgarzon · 2019-03-21T03:32:58Z

@Casz will take a look tomorrow, I feel responsible to get this fixed :)

jetersen · 2019-03-21T20:53:55Z

@dgarzon 👀 🙈

timja · 2019-03-22T07:32:21Z

.../src/main/java/io/jenkins/plugins/casc/impl/configurators/HeteroDescribableConfigurator.java

-            .collect(Collectors.toMap(
-                    d -> DescribableAttribute.getSymbols(d, api, target).get(0),
-                    d -> d.getKlass().toJavaClass()));
+    @SuppressWarnings("unused")


do you need this suppression given its used in tests?
if you do need one maybe VisibleForTesting would be better

timja · 2019-03-22T07:36:16Z

.../src/main/java/io/jenkins/plugins/casc/impl/configurators/HeteroDescribableConfigurator.java

+        return Option.of(Jenkins.getInstance().getPlugin(name)).isDefined();
+    }
+
+    private HashMap<String, Descriptor<T>> handleDuplicateSymbols(HashMap<String, Descriptor<T>> r, Tuple2<String, Descriptor<T>> t) {


it's not very clear what the parameters are here, what are r and t?

also do we need to be using HashMap or can we use Map?

timja · 2019-03-22T07:37:11Z

support/src/main/java/io/jenkins/plugins/casc/support/jobdsl/SeedJobConfigurator.java

    @Nonnull
    @Override
-    public List<Configurator> getConfigurators(ConfigurationContext context) {
+    public List<Configurator<GeneratedItems[]>> getConfigurators(ConfigurationContext context) {


looks unrelated?

No, it is very much related. The Configurator class was changed to actually need the type defined and not done through reflection. Hence the binary incompatibility fix.

timja

Code looks great, and nice test. Have asked a couple of question but no blockers

jetersen · 2019-03-28T05:03:52Z

@dgarzon 🙏

timja · 2019-03-29T14:34:34Z

@Casz this ready to go?

jetersen · 2019-03-29T19:02:50Z

It is, I just want @dgarzon to have look at it again with my additions 😅

dgarzon · 2019-03-29T19:04:53Z

Thanks @Casz for taking care of this! Looks good on my end :) Been super busy at work, hopefully I will free up a bit and be back here to contribute more often!

jetersen · 2019-03-29T19:10:00Z

🙌

BlueAndi · 2019-04-04T21:15:42Z

If the descriptor list of the describable attribute is empty, another problem raise up in the schema, because of the "oneOf"

...
  "schema" : {
    "oneOf": [
  <j:forEach items="${app.getDescriptorList(it.type)}" var="d" varStatus="st">
...

which expects at least one item.

jetersen · 2019-04-04T21:24:25Z

@BlueAndi wrap it in a app.getDescriptorList(it.type).size if statement?

BlueAndi · 2019-04-04T21:30:59Z

@Casz Sounds good, I will try. I saw that here the same problem appears: schema
Can I use it.implementors.entrySet().size here?

...
  "${it.target.name}": {
    "type": "object",
    "oneOf" : [
  <j:forEach items="${it.implementors.entrySet()}" var="impl" varStatus="st">
...

BlueAndi · 2019-04-05T21:51:22Z

Previous mentioned problem fixed in db44a1e

Regarding the

...
  "schema" : {
    "oneOf": [
  <j:forEach items="${app.getDescriptorList(it.type)}" var="d" varStatus="st">
...

I don't get the point about the whole construction, as I think "schema" misses a dollar sign as well.

timja approved these changes Mar 19, 2019

View reviewed changes

jetersen force-pushed the FEAT-hetero-describable-should-detect-duplicate-implementations-of-keys branch from 576249d to 30700cf Compare March 19, 2019 20:56

jetersen approved these changes Mar 19, 2019

View reviewed changes

jetersen requested a review from timja March 21, 2019 01:21

jetersen force-pushed the FEAT-hetero-describable-should-detect-duplicate-implementations-of-keys branch 2 times, most recently from 409352a to 1715987 Compare March 21, 2019 01:24

jetersen mentioned this pull request Mar 21, 2019

Race condition for dsl job queue() and dsl script security #619

Closed

jetersen force-pushed the FEAT-hetero-describable-should-detect-duplicate-implementations-of-keys branch from 1715987 to 52c0069 Compare March 21, 2019 03:27

timja reviewed Mar 22, 2019

View reviewed changes

timja approved these changes Mar 22, 2019

View reviewed changes

jetersen changed the title ~~FEAT: describable should warn on multiple implementations of a symbol.~~ feat: describable should warn on multiple implementations of a symbol. Mar 23, 2019

jetersen added the feature A PR that adds a feature - used by Release Drafter label Mar 24, 2019

jetersen force-pushed the FEAT-hetero-describable-should-detect-duplicate-implementations-of-keys branch 2 times, most recently from ac2cbc0 to 805b651 Compare March 29, 2019 00:09

FEAT: describable should warn on multiple implementations of a symbol.

988c471

jetersen force-pushed the FEAT-hetero-describable-should-detect-duplicate-implementations-of-keys branch from 805b651 to 021240b Compare March 29, 2019 01:23

fix binary compatibility in SeedJobConfigurator

54e2b59

jetersen force-pushed the FEAT-hetero-describable-should-detect-duplicate-implementations-of-keys branch from 021240b to 54e2b59 Compare March 29, 2019 01:29

jetersen changed the title ~~feat: describable should warn on multiple implementations of a symbol.~~ Describable will give a warning on multiple implementations of a symbol Mar 29, 2019

jetersen merged commit 63c82a8 into jenkinsci:master Mar 29, 2019

ingwarsw added a commit to ingwarsw/configuration-as-code-plugin that referenced this pull request Mar 30, 2019

Fix issue after merging jenkinsci#674 + tests fixes

e1e619d

BlueAndi mentioned this pull request Apr 5, 2019

fix JSON schema issues #778

Merged

jetersen mentioned this pull request Apr 10, 2019

HeteroDescribableConfigurator fails on duplicate keys. #658

Closed

3 tasks

mglazer mentioned this pull request Apr 15, 2019

Can't export configuration successfully without getting stack traces #832

Closed

3 tasks

Describable will give a warning on multiple implementations of a symbol #674

Describable will give a warning on multiple implementations of a symbol #674

Uh oh!

Conversation

dgarzon commented Dec 6, 2018

Uh oh!

jetersen commented Dec 7, 2018

Uh oh!

timja left a comment

Choose a reason for hiding this comment

Uh oh!

jetersen commented Mar 19, 2019

Uh oh!

dgarzon commented Mar 19, 2019

Uh oh!

jetersen commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jetersen commented Mar 19, 2019

Uh oh!

jetersen commented Mar 19, 2019

Uh oh!

BlueAndi commented Mar 19, 2019

Uh oh!

jetersen commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jetersen commented Mar 21, 2019

Uh oh!

jetersen commented Mar 21, 2019

Uh oh!

dgarzon commented Mar 21, 2019

Uh oh!

jetersen commented Mar 21, 2019

Uh oh!

timja Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timja Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

timja Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

jetersen Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

timja left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jetersen commented Mar 28, 2019

Uh oh!

timja commented Mar 29, 2019

Uh oh!

jetersen commented Mar 29, 2019

Uh oh!

dgarzon commented Mar 29, 2019

Uh oh!

jetersen commented Mar 29, 2019

Uh oh!

BlueAndi commented Apr 4, 2019

Uh oh!

jetersen commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BlueAndi commented Apr 4, 2019

Uh oh!

BlueAndi commented Apr 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jetersen commented Mar 19, 2019 •

edited

Loading

jetersen commented Mar 21, 2019 •

edited

Loading

timja Mar 22, 2019 •

edited

Loading

timja left a comment •

edited

Loading

jetersen commented Apr 4, 2019 •

edited

Loading