Skip to content
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

Remove plugin management #769

Merged
merged 18 commits into from Mar 18, 2019

Conversation

timja
Copy link
Member

@timja timja commented Mar 5, 2019

Here is our checklist for contributors. No hard requirement here, just a reminder

  • Please describe what you did
    Remove plugin feature

  • Link to issue you're working on if there's a relevant one
    Related to: allowRestart flag added #766

  • Did you provide a test-case to demonstrate feature actually works / issue is fixed ?
    Removed lots of them 😆

  • Please also consider adding a line to CHANGELOG.md

@timja timja mentioned this pull request Mar 5, 2019
4 tasks
@jetersen
Copy link
Member

jetersen commented Mar 5, 2019

I guess update sites is automatically moved to unclassified block? Which is fine, perhaps you could verify? 🤔

@timja
Copy link
Member Author

timja commented Mar 5, 2019

I guess update sites is automatically moved to unclassified block? Which is fine, perhaps you could verify? 🤔
Its in a a custom configurator so I would expect not, I didn't change anything there

I can't see any test case or demo for that feature.

Since you use it, what's the config you normally use, maybe you want to add a test and demo for it 😉?

Happy enough to add later on if you can send an example though

@jetersen
Copy link
Member

jetersen commented Mar 5, 2019

@timja rats 🙍‍♂️

@ewelinawilkosz
Copy link
Contributor

@Casz sorry, but I don't understand your comment :P is it ok to merge in your opinion?

@jetersen
Copy link
Member

jetersen commented Mar 5, 2019

He asked me to validate it and I would like to validate it quickly 😄 I'll merge once I can confirm that update site is possible to manage 😄 Or add a test even.

}

private UpdateCenter configureUpdateSites(Mapping map, Jenkins jenkins, ConfigurationContext context) throws ConfiguratorException {
final CNode sites = map.get("sites");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update site require some more code it seems 😄

Copy link
Member Author

@timja timja Mar 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah damn, happy to make it work if you push a test 😉

@jetersen jetersen force-pushed the feature/remove-plugin-management branch from 2be0d75 to 6a59308 Compare March 6, 2019 01:15
@jetersen
Copy link
Member

jetersen commented Mar 6, 2019

I rebased after the big #756 😄
Added test and configurator.

The configurator is added to jenkins root element, could not figure out how to add it to unclassified but tbh it might be better under jenkins 😄

@ndeloof @timja WDYT?

image

@timja
Copy link
Member Author

timja commented Mar 6, 2019

Thanks @Casz LGTM
just the two questions but I think this is fine to merge

@ewelinawilkosz
Copy link
Contributor

and +1 from me, I don't see a problem with updateCenter under jenkins root element

@timja
Copy link
Member Author

timja commented Mar 6, 2019

afaik all custom configurators have been put under the jenkins model

EDIT: not correct ignore me, seems fine

…onfiguration-as-code-plugin into feature/remove-plugin-management
@jetersen
Copy link
Member

jetersen commented Mar 6, 2019

I would like to fix up the proxy configuration too! This way we don't depreciate all features that were in plugin management but just move them from fancy management to configuration part.

@timja
Copy link
Member Author

timja commented Mar 6, 2019

I'm happy to do that part won't take long, unless you're already doing it?

@jetersen
Copy link
Member

jetersen commented Mar 6, 2019

@timja I welcome you, I am on a train to work 😄

@timja
Copy link
Member Author

timja commented Mar 6, 2019

same

@timja
Copy link
Member Author

timja commented Mar 6, 2019

Not quite sure what to put in the describe function, the test is currently failing and the export fails:

   5.155 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.password = password
   5.155 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.port = 80
   5.155 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.name = proxyhost
   5.155 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.testUrl = http://google.com
   5.156 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.userName = login
   5.156 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.noProxyHost = externalhost
   5.156 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting hudson.model.Hudson@3b603b4b.proxy = null
proxy: "FAILED TO EXPORT hudson.model.Hudson#proxy: \ncom.google.inject.ConfigurationException:\
  \ Guice configuration errors:\n\n1) Could not find a suitable constructor in hudson.ProxyConfiguration.\
  \ Classes must have either one (and only one) constructor annotated with @Inject\
  \ or a zero-argument constructor that is not private.\n  at hudson.ProxyConfiguration.class(ProxyConfiguration.java:81)\n\
  \  while locating hudson.ProxyConfiguration\n\n1 error\n\tat com.google.inject.internal.InjectorImpl.getProvider(InjectorImpl.java:1042)\n\
  \tat com.google.inject.internal.InjectorImpl.getProvider(InjectorImpl.java:1001)\n\
  \tat com.google.inject.internal.InjectorImpl.getInstance(InjectorImpl.java:1051)\n\
  \tat jenkins.ProxyInjector.getInstance(ProxyInjector.java:98)\n\tat io.jenkins.plugins.casc.core.JenkinsConfigurator.lambda$describe$5(JenkinsConfigurator.java:74)\n\
  \tat io.jenkins.plugins.casc.Attribute.getValue(Attribute.java:174)\n\tat io.jenkins.plugins.casc.Attribute.describe(Attribute.java:184)\n\
  \tat io.jenkins.plugins.casc.core.JenkinsConfigurator.describe(JenkinsConfigurator.java:92)\n\
  \tat io.jenkins.plugins.casc.core.ProxyConfiguratorTest.describeProxyConfig(ProxyConfiguratorTest.java:46)\n\
  \tat sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n\tat sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)\n\
  \tat sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)\n\
  \tat java.lang.reflect.Method.invoke(Method.java:498)\n\tat org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)\n\
  \tat org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)\n\
  \tat org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)\n\
  \tat org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)\n\
  \tat org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:553)\n\tat org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)\n\
  \tat org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)\n\
  \tat java.util.concurrent.FutureTask.run(FutureTask.java:266)\n\tat java.lang

will poke at it later today unless you know what's missing

@timja
Copy link
Member Author

timja commented Mar 7, 2019

@ndeloof or @Casz either of you got time to have a quick look and see what's wrong with the configurator?
It's currently logging:

   6.305 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.password = password
   6.306 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.port = 80
   6.306 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.name = proxyhost
   6.306 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.testUrl = http://google.com
   6.307 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.userName = login
   6.307 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting null.noProxyHost = externalhost
   6.307 [id=13]	INFO	i.jenkins.plugins.casc.Attribute#setValue: Setting hudson.model.Hudson@2917c73d.proxy = null

the null. is the owner being null but I think that's how these work?

Just need to run the ProxyConfiguratorTest

@timja
Copy link
Member Author

timja commented Mar 13, 2019

@Casz I made the proxy config work, although I couldn't get the export to work, I was getting stackoverflow errors...

Probably haven't implemented the configurator quite right, but given how the ProxyConfiguration code is structured in core it's not the easiest to make work...

I'll see if I have time to send a PR to core for improving it there

@timja timja force-pushed the feature/remove-plugin-management branch from bbe9c1e to 9b5ca8e Compare March 13, 2019 14:25
@jetersen
Copy link
Member

I'll give this PR a look to see if I can fix export, if not. I will go ahead and cut a release either way 😄

Does that sound like a deal @ewelinawilkosz ?

a default describe covers most scenarios for a configurator, if more complex cases are needed you would override the describe method
@@ -156,6 +157,15 @@ default Class getImplementedAPI() {
* Only export attributes which are <b>not</b> set to default value.
*/
@CheckForNull
CNode describe(T instance, ConfigurationContext context) throws Exception;
default CNode describe(T instance, ConfigurationContext context) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndeloof do you have anything against a default describe? This can be used for basic cases.

MavenConfigurator would need a custom but Jenkins, AdminWhitelistRuleConfigurator, UpdateCenterConfigurator, UpdateSiteConfigurator seems to settle for default case

@jetersen jetersen requested a review from ndeloof March 18, 2019 06:57
@timja
Copy link
Member Author

timja commented Mar 18, 2019

this good to go @Casz?

@jetersen
Copy link
Member

I think so, but I would love a comment from the original author whether a default describe is good or bad 🗡️ ping @ndeloof 😅

@jetersen
Copy link
Member

🤷‍♂️

@jetersen jetersen merged commit 6d5d625 into jenkinsci:master Mar 18, 2019
@jetersen
Copy link
Member

jetersen commented Mar 19, 2019

See [JENKINS-53767] Offer plugin management tooling
Reasoning for removing it can actually be found in this comment: #761 (comment)

tbh plugin management should be handled by Jenkins Core and provide a CLI interface and programmable java interface to manage. That's my two cents.

Then we could deprecate this whole plugin management mess located in this repo and the Jenkins docker repo with the whole install-plugin.sh 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants