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

[JENKINS-34996] Release parameters visibility #17

Merged
merged 3 commits into from Sep 28, 2016

Conversation

Projects
None yet
9 participants
@amuniz
Contributor

amuniz commented Jun 2, 2016

JENKINS-34996

Acknowledges SECURITY-170 (and parent pom upgrade, as it was needed to properly test

@reviewbybees

@rsandell

This comment has been minimized.

Show comment
Hide comment
@rsandell

rsandell Jun 3, 2016

Member

It looks like a bit of strange mix of solving the issue.

You could either skip adding the parameters and provide them via an EnvironmentContributor instead.

Or if, for legacy reasons with external scripts for example, you need the parameters still there you should use the new constructor added in 2.6 ParameterAction(parameters, additionalSafeParameters) - You could call that with reflection to not have to depend on a newer core.

Member

rsandell commented Jun 3, 2016

It looks like a bit of strange mix of solving the issue.

You could either skip adding the parameters and provide them via an EnvironmentContributor instead.

Or if, for legacy reasons with external scripts for example, you need the parameters still there you should use the new constructor added in 2.6 ParameterAction(parameters, additionalSafeParameters) - You could call that with reflection to not have to depend on a newer core.

@rsandell

This comment has been minimized.

Show comment
Hide comment
@rsandell

rsandell Jun 3, 2016

Member

Using a new ParametersAction will cause the xml api to be scewed anyway because it is not the old params in the xml api iirc.

Member

rsandell commented Jun 3, 2016

Using a new ParametersAction will cause the xml api to be scewed anyway because it is not the old params in the xml api iirc.

@amuniz

This comment has been minimized.

Show comment
Hide comment
@amuniz

amuniz Jun 3, 2016

Contributor

You could either skip adding the parameters and provide them via an EnvironmentContributor instead.

This causes regressions is the build page (missing parameters).

If [...] you need the parameters still there you should use the new constructor added in 2.6

Yeah, I considered this option but, if possible, I prefer to avoid the reflection dance (and the lapse of core versions where it won't work).

Using a new ParametersAction will cause the xml api to be scewed anyway because it is not the old params in the xml api iirc.

No, there are no XML API incompatibilities. The change is compatible as this ParametersAction override generates:

<action>
    <parameter>
        <name>MY_PARAM</name>
        <value>4567</value>
    </parameter>
</action>

Which is exactly the same than before (@Exported/@ExportedBean are inherited).

Contributor

amuniz commented Jun 3, 2016

You could either skip adding the parameters and provide them via an EnvironmentContributor instead.

This causes regressions is the build page (missing parameters).

If [...] you need the parameters still there you should use the new constructor added in 2.6

Yeah, I considered this option but, if possible, I prefer to avoid the reflection dance (and the lapse of core versions where it won't work).

Using a new ParametersAction will cause the xml api to be scewed anyway because it is not the old params in the xml api iirc.

No, there are no XML API incompatibilities. The change is compatible as this ParametersAction override generates:

<action>
    <parameter>
        <name>MY_PARAM</name>
        <value>4567</value>
    </parameter>
</action>

Which is exactly the same than before (@Exported/@ExportedBean are inherited).

</plugins>
</build>
<properties>
<jenkins.version>1.609.3</jenkins.version>

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

So it bumps the version. What's the reason? New API dependency?

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

So it bumps the version. What's the reason? New API dependency?

This comment has been minimized.

@amuniz

amuniz Jun 3, 2016

Contributor

No, it's common sense. 1.481 was not even Jenkins, and 1.609.3 is the oldest LTS in a 1 year window. So I think this bump is not going to affect anyone.

@amuniz

amuniz Jun 3, 2016

Contributor

No, it's common sense. 1.481 was not even Jenkins, and 1.609.3 is the oldest LTS in a 1 year window. So I think this bump is not going to affect anyone.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

It's debatable. My previous instance still works on 1.509.4, @olivergondza uses 1.580.x IIRC.
For enterprise installations it's common to be behind the trends due to [stabilization/migration testing] efforts.

So I would ask the plugin maintainer

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

It's debatable. My previous instance still works on 1.509.4, @olivergondza uses 1.580.x IIRC.
For enterprise installations it's common to be behind the trends due to [stabilization/migration testing] efforts.

So I would ask the plugin maintainer

This comment has been minimized.

@amuniz

amuniz Jun 3, 2016

Contributor

Ok.

@avalanche123 not sure if you are the current maintainer but you seem to have done the last releases. Are you acting as maintainer?

@amuniz

amuniz Jun 3, 2016

Contributor

Ok.

@avalanche123 not sure if you are the current maintainer but you seem to have done the last releases. Are you acting as maintainer?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 29, 2016

Member

During the merge I decided to go forward with 1.609.3. It is reasonable in this case, because the plugin needs to pick some new APIs for security reasons. This version is 1.5 years old and it still supports Java 6.

@oleg-nenashev

oleg-nenashev Sep 29, 2016

Member

During the merge I decided to go forward with 1.609.3. It is reasonable in this case, because the plugin needs to pick some new APIs for security reasons. This version is 1.5 years old and it still supports Java 6.

@@ -43,6 +38,7 @@
<dependency>
<groupId>org.jenkins-ci.main</groupId>
<artifactId>maven-plugin</artifactId>
<version>2.13</version>

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

Spaces vs. tabs

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

Spaces vs. tabs

This comment has been minimized.

@amuniz

amuniz Jun 3, 2016

Contributor

They are already mixed in this plugin, I just used spaces.

@amuniz

amuniz Jun 3, 2016

Contributor

They are already mixed in this plugin, I just used spaces.

import hudson.model.TaskListener;
@Restricted(NoExternalUse.class)
public class SafeParametersAction extends ParametersAction {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

I would not do it public at all. Restricted does not protect from misusages in Pipelines and system groovy scripts

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

I would not do it public at all. Restricted does not protect from misusages in Pipelines and system groovy scripts

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

NIT in any case

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

NIT in any case

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

I'm aware that SafeParametersAction does not actually override all methods using parameters in 1.609.3. E.g. createBuildWrappers, createVariableResolver, etc. I am not sure such change covers all use-cases.

Maybe it applies to my fix in jenkinsci/promoted-builds-plugin#93

Member

oleg-nenashev commented Jun 3, 2016

I'm aware that SafeParametersAction does not actually override all methods using parameters in 1.609.3. E.g. createBuildWrappers, createVariableResolver, etc. I am not sure such change covers all use-cases.

Maybe it applies to my fix in jenkinsci/promoted-builds-plugin#93

@amuniz

This comment has been minimized.

Show comment
Hide comment
@amuniz

amuniz Jun 3, 2016

Contributor

I'm aware that SafeParametersAction does not actually override all methods using parameters in 1.609.3. E.g. createBuildWrappers, createVariableResolver.

I don't see uses of that methods in the scope of this plugin, and any external call must follow the rules of ParametersAction - except for getParameters and getParameter(String).

Moreover, I've added test (if you think there is some uncovered case, I'm ok to add it) and I've tested it manually (including an upgrade).

So, if you still feel fear, what could I do? 😄

Contributor

amuniz commented Jun 3, 2016

I'm aware that SafeParametersAction does not actually override all methods using parameters in 1.609.3. E.g. createBuildWrappers, createVariableResolver.

I don't see uses of that methods in the scope of this plugin, and any external call must follow the rules of ParametersAction - except for getParameters and getParameter(String).

Moreover, I've added test (if you think there is some uncovered case, I'm ok to add it) and I've tested it manually (including an upgrade).

So, if you still feel fear, what could I do? 😄

@@ -1,3 +1,4 @@
<?jelly escape-by-default='true'?>

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

NIT: This file does not need to be Jelly at all. Not related to this PR

@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

NIT: This file does not need to be Jelly at all. Not related to this PR

This comment has been minimized.

@amuniz

amuniz Jun 3, 2016

Contributor

Mmm, I think InjectedTest was complaining about the lack of this line.

@amuniz

amuniz Jun 3, 2016

Contributor

Mmm, I think InjectedTest was complaining about the lack of this line.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Jun 3, 2016

Member

So, if you still feel fear, what could I do?

At least the fix does not decrease the technical debt (it fixes some cases at least), so I'm not going to force any major rework in this PR. My suggestion would be to migrate Jenkins core code to getParameters() getter, for example. CC @rsandell

Member

oleg-nenashev commented Jun 3, 2016

So, if you still feel fear, what could I do?

At least the fix does not decrease the technical debt (it fixes some cases at least), so I'm not going to force any major rework in this PR. My suggestion would be to migrate Jenkins core code to getParameters() getter, for example. CC @rsandell

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Jun 16, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

reviewbybees commented Jun 16, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@amuniz

This comment has been minimized.

Show comment
Hide comment
@amuniz
Contributor

amuniz commented Jul 27, 2016

@oleg-nenashev @rsandell ping?

@sevilS

This comment has been minimized.

Show comment
Hide comment
@sevilS

sevilS Aug 1, 2016

I think that @Fiouz, @avalanche123 are the most recent maintainers of release-plugin repo. ¿There is any plan about merge this pull request?

sevilS commented Aug 1, 2016

I think that @Fiouz, @avalanche123 are the most recent maintainers of release-plugin repo. ¿There is any plan about merge this pull request?

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Aug 2, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

reviewbybees commented Aug 2, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@recampbell

This comment has been minimized.

Show comment
Hide comment
@recampbell

recampbell Aug 17, 2016

Member

Perhaps @oleg-nenashev and @rsandell can review now that they're back from vacations.

Member

recampbell commented Aug 17, 2016

Perhaps @oleg-nenashev and @rsandell can review now that they're back from vacations.

@raspy

This comment has been minimized.

Show comment
Hide comment
@raspy

raspy Aug 18, 2016

Member

When can we expect plugin release with this fix? This has been impacting our release procedures for quite some time now.

Member

raspy commented Aug 18, 2016

When can we expect plugin release with this fix? This has been impacting our release procedures for quite some time now.

@zeraturl

This comment has been minimized.

Show comment
Hide comment
@zeraturl

zeraturl Sep 19, 2016

Hi, can you estimate when you merge the fix and release a new plugin version?

Or can you descripe how to deploy the fix of the plugin to a running jenkins server?
Thx.

zeraturl commented Sep 19, 2016

Hi, can you estimate when you merge the fix and release a new plugin version?

Or can you descripe how to deploy the fix of the plugin to a running jenkins server?
Thx.

@noppelmax

This comment has been minimized.

Show comment
Hide comment
@noppelmax

noppelmax Sep 27, 2016

With this pull request the release plugin is working again for me. @zeraturl you may run git pull origin pull/17/head and mvn package. This gives you the .hpi which can be loaded into jenkins. Works for me. Will jump back to official builds if this one is in.

noppelmax commented Sep 27, 2016

With this pull request the release plugin is working again for me. @zeraturl you may run git pull origin pull/17/head and mvn package. This gives you the .hpi which can be loaded into jenkins. Works for me. Will jump back to official builds if this one is in.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Sep 28, 2016

Member

Well, @avalanche123 does not seem to be active on GitHub. I'll go forward and release this fix

Member

oleg-nenashev commented Sep 28, 2016

Well, @avalanche123 does not seem to be active on GitHub. I'll go forward and release this fix

@oleg-nenashev oleg-nenashev merged commit ab68ac9 into jenkinsci:master Sep 28, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment