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

add junit test for stash file parameter #7

Merged
merged 34 commits into from
Jan 30, 2021
Merged

add junit test for stash file parameter #7

merged 34 commits into from
Jan 30, 2021

Conversation

olamy
Copy link
Member

@olamy olamy commented Jan 21, 2021

No description provided.

@olamy olamy added the enhancement New feature or request label Jan 21, 2021
@olamy olamy requested a review from jglick January 22, 2021 11:12
@olamy
Copy link
Member Author

olamy commented Jan 22, 2021

@jglick please have a look there is a bit of testing and documentation.
Maybe we can merge that and have a first release? please let me know.
Thanks

pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stuff to work with here. I would want to do some fixups before merging.

As to a release, I think it is prudent to look into input / build first. Also there is an unimplemented method. Need to look over the list of outstanding items, implement the easy ones, and refile the rest as GH issues.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
src/main/webapp/help-base64File.html Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
stages {
stage('Example') {
steps {
withFileParameter('FILE') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC Declarative supports using block-scoped steps in an option block. Does it work here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep it works

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you prefer to demonstrate using a block-scoped step inside steps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uhm I missed that one but I'm not sure what sort of syntax you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like

pipeline {
  agent any
  parameters {
    base64File(name: 'FILE')
  }
  options {
    withFileParameter('FILE')
  }
  stages {
    stage('x') {
      steps {
        sh 'cat $FILE'
      }
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug in Declarative it seems: https://issues.jenkins.io/browse/JENKINS-64761

olamy and others added 7 commits January 23, 2021 14:05
Co-authored-by: Jesse Glick <[email protected]>
Co-authored-by: Jesse Glick <[email protected]>
Co-authored-by: Jesse Glick <[email protected]>
Co-authored-by: Jesse Glick <[email protected]>
Signed-off-by: olivier lamy <[email protected]>
@olamy
Copy link
Member Author

olamy commented Jan 23, 2021

@jglick looks good to me to look into build/input. we can merge this PR then I can take care of it.

jglick
jglick previously requested changes Jan 23, 2021
README.md Outdated
stages {
stage('Example') {
steps {
withFileParameter('FILE') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you prefer to demonstrate using a block-scoped step inside steps?

olamy and others added 2 commits January 24, 2021 19:11
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I must have been tired!

README.md Outdated Show resolved Hide resolved
@@ -4,7 +4,7 @@
pipeline {
agent any
parameters {
stashed64File(name: 'FILE-STASH')
stashedBase64File(name: 'FILE-STASH')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
stashedBase64File(name: 'FILE-STASH')
stashedFile(name: 'FILE-STASH')

or, if it works,

Suggested change
stashedBase64File(name: 'FILE-STASH')
stashedFile 'FILE-STASH'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple syntax not working

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in another PR

@olamy olamy requested a review from jglick January 25, 2021 22:26
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry the back-and-forth across time zones is a little crazy!

README.md Outdated
stages {
stage('Example') {
steps {
withFileParameter('FILE') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like

pipeline {
  agent any
  parameters {
    base64File(name: 'FILE')
  }
  options {
    withFileParameter('FILE')
  }
  stages {
    stage('x') {
      steps {
        sh 'cat $FILE'
      }
    }
  }
}

@olamy
Copy link
Member Author

olamy commented Jan 27, 2021

Sorry the back-and-forth across time zones is a little crazy!

no worries especially with adding weekend, public holidays etc... 😄

@jglick jglick dismissed their stale review January 30, 2021 18:08

presumably stale

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enough to merge, but I have various follow-ups and corrections.

- [ ] inline help text
- [ ] `withFileParameter` compatibility
- [X] inline help text
- [X] `withFileParameter` compatibility (manual test)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are using withFileParameter then you are not supposed to need unstash (and vice-versa).

- [ ] inline help text
- [ ] `withFileParameter` compatibility
- [X] inline help text
- [X] `withFileParameter` compatibility (manual test)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [X] `withFileParameter` compatibility (manual test)
- [ ] `withFileParameter` compatibility

@@ -0,0 +1,19 @@
<div>
<p>File parameter compatible with Pipeline but this one stash the file and must be used to handle bug files, how to use it in a declarative pipeline:</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p>File parameter compatible with Pipeline but this one stash the file and must be used to handle bug files, how to use it in a declarative pipeline:</p>
<p>File parameter compatible with Pipeline but this one stash the file and must be used to handle big files, how to use it in a declarative pipeline:</p>

" stage('Example') {\n" +
" steps {\n" +
" withFileParameter('FILE') {\n" +
" echo(/loaded '${readFile(FILE).toUpperCase(Locale.ROOT)}' from $FILE/) \n" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW while sh 'cat $FILE' is most realistic for docs, I prefer readFile for functional tests because it requires no special work to pass on Windows.

r.assertLogContains("loaded 'UPLOADED CONTENT HERE'", b);
}

@Test public void stashed() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine but it is not using FileParameterWrapper so does not belong here. (Unless the two are made to be compatible and the test made to exercise that.)

r.assertLogContains("loaded 'UPLOADED CONTENT HERE'", b);
}

@Test public void stashedDeclarativeParameterCreated() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants