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

[WiP] Build data sucks #313

Closed
wants to merge 18 commits into from

Conversation

Projects
None yet
4 participants
@ndeloof
Copy link
Member

ndeloof commented Mar 24, 2015

Replace per-build BuildData with a job-centric BuiltRevisionMap to avoid data duplication
drawbacks:

  • drop support for scm names
  • drop support for BuildData related to another repository (sic)
  • still some test failure I need to investigate
@ExportedBean
public class BuiltRevisionMap implements Action, Saveable {

public static final String FILE = BuiltRevisionMap.class.getName() + ".xml";

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Mar 24, 2015

Member

In my new plugin i decided to use .runtime.xml file mask for all runtime data to not track constantly changed data (should be useful for example for jobConfigHistory).

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Mar 24, 2015

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

final BuildData buildData = fixNull(getBuildData(lastBuild));
if (buildData.lastBuild != null) {
listener.getLogger().println("[poll] Last Built Revision: " + buildData.lastBuild.revision);
BuiltRevisionMap builtRevisions = BuiltRevisionMap.forProject(project);

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Mar 24, 2015

Member

Why do you need TransientProjectActionFactory if you are getting Action directly from static class?

This comment has been minimized.

Copy link
@ndeloof

ndeloof Mar 24, 2015

Author Member

Doing this would load from xml any time you request BuiltRevisionMap for a project. TransientProjectActionFactory do attach to project in memory once for all

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Mar 24, 2015

Member

Sorry could you describe what do you mean about memory? As i see AbstractProject loads Action and there should be some interfaces to get this Action...

This comment has been minimized.

Copy link
@ndeloof

ndeloof Mar 24, 2015

Author Member

Action attached to job are saved in job config.xml. This action has it's own storage, so is tied to a job (so can be retrieved using Job.getAction(class) ) using a TransientActionFactory that add action to job without reference in persisted config.xml

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Mar 24, 2015

Member

Yes, real Actions are stored, Transient in-memory. But transient can be persisted.
AbstractProject has method protected List<Action> createTransientActions() { that should provide transient action through job api... But as i see it used only in matrix project and this weird, why not provide API for getting TransientAction from job related classes? (TransientProjectActionFactory provides the same data as direct static call)

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Mar 25, 2015

Member

And i was wrong, BuiltRevisionMap builtRevisions = AbstractProject.getAction(BuiltRevisionMap.class);should work

@KostyaSha

This comment has been minimized.

Copy link
Member

KostyaSha commented Mar 24, 2015

Btw, what will happen with plugins that operates directly with BuildData?

@ndeloof

This comment has been minimized.

Copy link
Member Author

ndeloof commented Mar 24, 2015

For existing APIs I've maintained some BuildData support, but for sure some exotic usage may be broken.


@Override
public Collection<? extends Action> createFor(AbstractProject target) {
if (target.getScm() instanceof GitSCM)

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Mar 25, 2015

Member

Will it work with multiple scm?

This comment has been minimized.

Copy link
@ndeloof

ndeloof Mar 25, 2015

Author Member

No
please remember this is WiP
also, I hate multiple-scm. This plugin do impact all SCM plugin and introduce dependency on "a proof-of-concept than a robust and fully functional component". Such a plugin should not exist and would better have been designed as some API enhancement in jenkins-core.

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Mar 25, 2015

Member

Sure, but now there is no other way :(

This comment has been minimized.

Copy link
@ndeloof

ndeloof Mar 25, 2015

Author Member

There is one : create a better solution (sic), deprecate this plugin with some migration code, remove all related code from scm plugins.

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Mar 25, 2015

Member

multiple scm allows to use not only git AFAIR. This should go to core and probably unhardcode sequence :) and maybe move SCM to builder :)

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Dec 8, 2018

See the results and discussion in #578

@MarkEWaite MarkEWaite closed this Dec 8, 2018

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.