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

[JENKINS-17680] Upgrade configurations from version 1.25 #23

Merged
merged 7 commits into from Aug 8, 2013

Conversation

@ikedam
Copy link
Member

commented Jul 27, 2013

Upgrading CopyArtfifact from 1.25 or earlier to 1.26 or later causes job names to copy from get empty in job configuration pages.
Running a build upgrades the configuration, but the configuration breaks if one opens the configuration page and save it before running a build.

With this patch, configurations are upgraded when they are loaded.

This works as following:

  • When a instance of CopyArtifact is unmarshalled, tests whether upgrade is needed.
  • If any instance of CopyArtifact are needed to be upgrade, scan for all jobs and perform upgrade all CopyArtifact.

Points are:

  • I did not persist the flag whether upgrading all CopyArtifact is done, for the case that a job with a old version of CopyArtifact configuration is recovered from a backup.
  • I did not remove existing codes that upgrade configurations when running a build. There may be a case scanning for CopyArtifact fails. For example, CopyArtifact is wrapped by another Builder.
@buildhive

This comment has been minimized.

Copy link

commented Jul 27, 2013

Jenkins » copyartifact-plugin #50 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Jul 27, 2013

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2013

I could not reproduce the test failure.
It happens in new CLI(getURL()), and seems not caused by my change.


boolean isUpgraded = false;
for (Project<?,?> project: Jenkins.getInstance().getAllItems(Project.class)) {
for (CopyArtifact target: Util.filter(project.getBuilders(), CopyArtifact.class)) {

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2013

Member

This will not work for a MatrixProject. Use getCopiers instead. (Which reminds me why the call to upgradeIfNecessary will still be necessary from perform: in case the build step is part of an AbstractProject which is neither a Project nor a MatrixProject.)

* Upgrade configuration of CopyArtifact after all jobs are loaded.
*/
@Extension
public class CopyArtifactUpgradeListener extends ItemListener {

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2013

Member

Mild preference for using @Init for this kind of thing, since it allows the upgrade to run in parallel with other initialization tasks.

}
int i = projectName.lastIndexOf('/');

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2013

Member

BTW a tip for minimizing diff lines in a PR: avoid changing indentation of long blocks of existing code if it can be helped:

    boolean upgradeIfNecessary(AbstractProject<?,?> job) throws IOException {
        if (isUpgradeNeeded()) {
            int i = projectName.lastIndexOf('/');
            …
            return true;
        } else {
            return false;
        }
    }

This practice makes the PR easier to review, makes history easier to follow (especially from git blame), and makes merge conflicts less likely.

@buildhive

This comment has been minimized.

Copy link

commented Aug 3, 2013

Jenkins » copyartifact-plugin #52 SUCCESS
This pull request looks good
(what's this?)

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2013

@jglick Thank you for your reviewing. I updated the request.

  • Use @Initialize instead of ItemListener.onLoad
  • Use getCopiers to be able to apply to MatrixProject.
    • Added a test to verify it works properly with MatrixProject.

And I added a test testWrappedCopierProjectNameSplit for upgradeIfNecessary from perform.

I made so many changes that commit logs are dirty.
Should I create another pull request for merging after reviewing?

((MatrixProject)project).getBuildersList() : null);
if (list == null) return Collections.emptyList();
List<CopyArtifact> copiers = list.getAll(CopyArtifact.class);
List<CopyArtifact> copiers = CopyArtifact.getCopiers(project);

This comment has been minimized.

Copy link
@ikedam

ikedam Aug 3, 2013

Author Member

Existing getCopiers is CopyArtifact$ListenerImpl.getCopiers.
I extracted partially as CopyArtifact.getCopiers.

This comment has been minimized.

Copy link
@jglick

jglick Aug 5, 2013

Member

Makes sense, though to reduce confusion it would be wise to pick different names for the two methods.

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2013

Changes in testProjectNameSplit.zip:

  • added matrix-copier that is MatrixProject and uses CopyArtifact.

new file testWrappedCopierProjectNameSplit.zip:

  • A project using CopyArtifact wrapped by another builder.
    • This is a case with [Conditional BuildStep Plugin|https://wiki.jenkins-ci.org/display/JENKINS/Conditional+BuildStep+Plugin]
assertFalse(configXml, configXml.contains("<projectName>"));
// When a project is specified with a variable, it is split improperly.
assertTrue(configXml, configXml.contains("<project>matrix</project>"));
assertTrue(configXml, configXml.contains("<parameters>which=${which}</parameters>"));

This comment has been minimized.

Copy link
@ikedam
@jglick

This comment has been minimized.

Copy link
Member

commented Aug 5, 2013

Should I create another pull request for merging after reviewing?

I see no need to rebase this way. Who cares how many commits it took so long as the end result is right?

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2013

@jglick Thanks for your reviewing.
I fixed what you pointed.

ikedam added a commit that referenced this pull request Aug 8, 2013
[JENKINS-17680] Upgrade configurations from version 1.25
@ikedam ikedam merged commit f13ba75 into jenkinsci:master Aug 8, 2013
@buildhive

This comment has been minimized.

Copy link

commented Aug 8, 2013

Jenkins » copyartifact-plugin #55 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2013

Hmmm.... Why this failure is reproduced?

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2013

@jglick

This comment has been minimized.

BTW now that the names are unique, the CopyArtifact. disambiguating prefix can be dropped here.

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2013

Thank you.
I also found unnecessary import, fixed them in f468296 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.