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-38110] Add a libraries section #128

Merged
merged 4 commits into from Mar 4, 2017
Merged

Conversation

@abayer
Copy link
Member

abayer commented Mar 1, 2017

  • JENKINS issue(s):
  • Description:
    • Initial work on adding a libraries section to Declarative. Current syntax looks like:
libraries {
    lib('foo@1.2.3')
    lib('bar')
}

Yes, I'd prefer not to have that pointless lib there, but the alternative is something like libraries 'foo@1.2.3', 'bar' and as we may remember from agent back in the day, I do not care for that syntax at all. Plus, we may end up wanting to include the ability to specify a LibraryRetriever (see LibraryRetriever.java in workflow-cps-global-lib for details), in which case going to lib(identifier:'foo@1.2.3', retriever:[$class:...]) is a much better option than anything else I could find.

Note that this is very preliminary at this point - like, it doesn't
actually call the library step yet, since we don't yet have a release
of workflow-cps-global-lib-plugin with that included to depend on. We
also don't have parsing tests or any of that jazz. But that's ok.
@abayer abayer added this to the 1.1 milestone Mar 1, 2017
@abayer
Copy link
Member Author

abayer commented Mar 1, 2017

Note that this is very preliminary at this point - like, it doesn't
actually call the library step yet, since we don't yet have a release
of workflow-cps-global-lib-plugin with that included to depend on. We
also don't have parsing tests or any of that jazz. But that's ok.

@kzantow
Copy link

kzantow commented Mar 1, 2017

Could the LibraryRetriever have some special metadata that could list available libraries in some cases? I guess we'd just allow a random string for now or am I missing something?

@HRMPW
Copy link

HRMPW commented Mar 1, 2017

@kzantow LibraryRetriever lets you load a library from an arbitrary source. lib itself could possibly know the list of available libraries based on the configuration in the folder or global but I don't think you can pre-populate LibraryRetriever

@abayer
Copy link
Member Author

abayer commented Mar 1, 2017

Yeah, the LibraryRetriever could specify things like credentials, an alternate repo (since it'd be GitHub by default), etc... I'm also trying to figure out what to do about validation of the library itself - it looks like I'd need the job to be able to validate the identifier string if folder context needs to be taken into account.

@abayer
Copy link
Member Author

abayer commented Mar 1, 2017

@HRMPW LibraryStep allows an optional LibraryRetriever argument. Assuming we do add support for it, the syntax would probably be lib("foo@1.2.3") or lib("foo@1.2.3", modernSCM([$class: 'GitSCMSource', credentialsId: '', excludes: '', id: 'id', ignoreOnPushNotifications: true, includes: '*', remote: 'https://nowhere.net/'])). Ideally, we'll get @Symbols and proper DataBoundConstructor setups on the SCMSource implementations to make that syntax smoother, though.

@kzantow
Copy link

kzantow commented Mar 1, 2017

Sorry, was conflating things... my goal for the editor is - as much as possible - to give the user choices for things to minimize errors, so as much info as we can pull from jenkins, the better... e.g. which LibraryRetriever types are present, possibly what they resolve, etc., and any known entries for the lib itself.

@abayer
Copy link
Member Author

abayer commented Mar 1, 2017

@kzantow Yup, that we can do to at least some extent - leveraging LibraryStep's auto-complete logic

@jglick
Copy link
Member

jglick commented Mar 1, 2017

we don't yet have a release of workflow-cps-global-lib-plugin with that included to depend on

Try: 2.7-20170301.201004-1

@abayer
Copy link
Member Author

abayer commented Mar 1, 2017

Thanks, @jglick - done.

@abayer abayer removed the work-in-progress label Mar 2, 2017
@abayer
Copy link
Member Author

abayer commented Mar 2, 2017

Moving out of work-in-progress - decided to leave out LibraryRetriever for the initial release and wait to see if there's demand.

@reviewbybees
Copy link

reviewbybees commented Mar 2, 2017

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.

@rsandell
Copy link
Member

rsandell commented Mar 3, 2017

If we go with adding folder context to the validation we should also revisit the validation of the credentials function in environment so we can fail there as well fast if the user has entered an non existent credential id.

Copy link
Member

rsandell left a comment

🐝

So I'm guessing we deliberately don't support static imports from the dynamic libraries.

import org.jenkinsci.plugins.workflow.cps.CpsScript

/**
* Translates a closure containing a sequence of "library('string')" calls into an instance of {@link Libraries}.

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 3, 2017

Member

Wasn't it lib('string')?

This comment has been minimized.

Copy link
@abayer

abayer Mar 3, 2017

Author Member

Fixed!

@jglick
jglick approved these changes Mar 3, 2017
"description": "One or more shared library identifiers to load",
"type": "array",
"items": {
"type": "string"

This comment has been minimized.

Copy link
@jglick

jglick Mar 3, 2017

Member

How would you compatibly extend this to support LibraryRetriever?

This comment has been minimized.

Copy link
@abayer

abayer Mar 3, 2017

Author Member

By supporting both the current array-of-strings and a new format for LibraryRetriever - anyOf will let us do that easily enough.

pom.xml Outdated
@@ -148,7 +148,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-cps-global-lib</artifactId>
<version>2.5</version>
<version>2.7-20170301.201004-1</version> <!-- TODO: Update to 2.7 once released -->

This comment has been minimized.

Copy link
@jglick

jglick Mar 3, 2017

Member

Released.

This comment has been minimized.

Copy link
@abayer

abayer Mar 3, 2017

Author Member

Updated.

@jglick
jglick approved these changes Mar 3, 2017
@abayer abayer merged commit 0c7d4fd into jenkinsci:master Mar 4, 2017
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.