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

Provide the data about parent builds as environment variable to the dependent build #319

Merged
merged 1 commit into from Dec 1, 2017

Conversation

Projects
None yet
3 participants
@Jimilian
Copy link
Contributor

commented Jul 25, 2017

Idea is quite simple: we have build and test jobs and we want to get rid of intermediate steps. So, test job has build job as dependency and know how to download artifacts.
Other possible use cases:

  • build some shared code once and re-use in dependent builds
  • re-trigger only small pieces
  • provide more granular feedback

@Jimilian Jimilian force-pushed the Jimilian:dependency branch 2 times, most recently from b833768 to 3bf1a05 Jul 25, 2017

@rsandell
Copy link
Member

left a comment

Add a JenkinsRule based test as well please, so we can verify the integration works.

String prefix = "DEP_" + keyName;
String number = String.valueOf(run.getNumber());
String result = run.getResult().toString();
parameters.add(new StringParameterValue(prefix + "_BUILD_NAME", originalName));

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 4, 2017

Member

This won't work after the introduction of SECURITY-170. Core will reject these parameters.

Add an EnvironmentContributingAction to the build instead of build parameters.

The build parameters was only kept in this plugin due to backwards compatibility. Any new stuff not related to the event itself we should try to provide the more "correct way".

This comment has been minimized.

Copy link
@Jimilian

Jimilian Aug 4, 2017

Author Contributor

Thx, will do

This comment has been minimized.

Copy link
@Jimilian

Jimilian Aug 9, 2017

Author Contributor

@rsandell, I did it... and only after that I faced the problem. This Action doesn't support Pipeline jobs... https://issues.jenkins-ci.org/browse/JENKINS-29537

@Jimilian

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2017

Filled jenkinsci/jenkins#2975 as well.

* Adds Action that stores the data about dependency jobs.
*/
public class GerritDependencyAction extends InvisibleAction implements EnvironmentContributingAction {
private List<Run> runs = Collections.emptyList();

This comment has been minimized.

Copy link
@jglick

jglick Aug 24, 2017

Member

If this action is ever serialized to disk anywhere, which I suppose it is (seems to be added to queue.xml and EnvironmentContributingAction is added to `build.xml), then this field is illegal. It will in fact explode on you: jenkinsci/jenkins#2957

Maybe you just want a List<String> of ids?

@Jimilian Jimilian force-pushed the Jimilian:dependency branch 2 times, most recently from ce20320 to 1cc8fb8 Sep 1, 2017

@Jimilian

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2017

@rsandell, @jglick, please, review

@Jimilian

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2017

@rsandell, @jglick, could you take a look, please?

@rsandell
Copy link
Member

left a comment

Sorry for the review delay, I thought the upstream changes in core where still in limbo.

depKeys.append(" ");
}

env.put("DEP_KEYS", depKeys.toString().trim());

This comment has been minimized.

Copy link
@rsandell

rsandell Sep 29, 2017

Member

is DEP_KEYS really a good name?

This comment has been minimized.

Copy link
@Jimilian

Jimilian Sep 29, 2017

Author Contributor

I did not want to put GERRIT_ prefix in front to distinguish plugin "features" and data that comes from Gerrit. DEP_KEYS works fine for me, but we can rename it to something better. Any suggestions?

This comment has been minimized.

Copy link
@rsandell

rsandell Sep 29, 2017

Member

Yea, that's a good distinction to have; that GERRIT_* is data about the event from Gerrit. I'll need to remember that :)
At the same time I think it need to be a bit more distinguishable from any other env var in the mix.

I'm not sure but maybe I just don't like the unnecessary abbreviation of dependency.

Perhaps somewhat mimicing what the Parameterized Trigger Plugin provides would be OK? But perhaps not exactly.

TRIGGER_DEPENDENCY_KEYS ?

This comment has been minimized.

Copy link
@rsandell

rsandell Sep 29, 2017

Member

And perhaps prefixing the others with TRIGGER_ as well?

This comment has been minimized.

Copy link
@Jimilian

Jimilian Oct 4, 2017

Author Contributor

I like this idea. Will switch to TRIGGER_.

@@ -188,18 +188,40 @@ public CauseOfBlockage canRun(Queue.Item item) {
return new BecauseDependentBuildIsBuilding(blockingProjects);
} else {
logger.info("No active dependencies on project: {} , it will now build", p);

ToGerritRunListener toGerritRunListener = ToGerritRunListener.getInstance();

This comment has been minimized.

Copy link
@rsandell

rsandell Sep 29, 2017

Member

The above logging statement is a bit misleading, probably leading to this.
But returning null from a QueueTaskDispatcher does not necessarily mean that the build will run. It only means that this dispatcher thinks it's OK for the build to start. There could be other dispatchers later in the chain that will block it still.
And that means that this code can be called several times for an item in the queue and you'll end up adding multiple GerritDependencyActions to the build.
Using either a TransientActionFactory or maybe a QueueListener would be better. You could also potentially add the one action to it when the trigger actually schedules the build and let it calculate itself when needed.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Oct 4, 2017

Author Contributor

Can I use actions.replace for now? I think the list of actions for Queue item should be relatively small (did not measure, btw). So, it will not be very expensive. In this case the rest of code will be the same.
Also it looks like in case of multiple actions we always pickup the first one, so, it means if we have the dependency A-B, we can have the race:

  • A passed. This dispatcher added action.
  • B was blocked because of other QueueTaskDispatcher
  • A was re-triggered
  • A passed, another dispatcher said ok
  • Looks like we should see the first build of A - not a last one.

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 12, 2017

Member

Actionable has an addOrReplace that could work for now. But please add a TODO as well that this needs to be fixed.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Oct 30, 2017

Author Contributor

It's added only in 2.29, but replaceAction called it inside anyway.

public GerritDependencyAction(List<Run> runs) {
deps = new ArrayList<String>(runs.size());
for (Run run : runs) {
deps.add(run.getParent().getFullName() + "#" + run.getNumber() + "#" + run.getResult());

This comment has been minimized.

Copy link
@rsandell

rsandell Sep 29, 2017

Member

See if you can use getExternalizableId

This comment has been minimized.

Copy link
@Jimilian

Jimilian Oct 30, 2017

Author Contributor

It's too slow (because it uses the same getItemByFullname) and we want to keep the data about build result even if parent build was removed only.

@Jimilian Jimilian force-pushed the Jimilian:dependency branch 2 times, most recently from fe6000d to bff75dc Oct 30, 2017

JENKINS-25983: Provide variable to dependent builds
- Use GerritDependencyAction to follow SECURITY-170
- Use `Actionable.replaceAction`, because `addOrReplace` is added only
in 2.29
- Do not use `getExternalizableId` to provide the data about parents
even for obsolete parents
- Stop to scan triggered builds once first blocker is found

@Jimilian Jimilian force-pushed the Jimilian:dependency branch from bff75dc to 35c1499 Nov 20, 2017

@rsandell rsandell merged commit c18db69 into jenkinsci:master Dec 1, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@Jimilian

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2017

🕺

@Jimilian Jimilian deleted the Jimilian:dependency branch Dec 1, 2017

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