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

Allow custom text for abort - just like the ok (proceed button) #54

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gyula-s
Copy link

@gyula-s gyula-s commented Oct 1, 2021

Currently we are allowed to specify a custom text for the proceed button, but not for the abort.
Recently I had a project, where I was asking the user a question, and the "Abort" wasn't really aborting a build, and the word "Abort" didn't really make sense in the context it was used in.

This pr is to allow the users to specify the text on this button.

  • I didn't find where should I open an issue... if you can create one and link this pr to it, that would be appreciated.
  • I didn't see any tests, that would test the ok (proceed) text change, so I didn't add any for the abort either. I'm not a Java dev, I'm happy that I was able to do this in the first place.
  • I couldn't find where can I leave a pr for the documentation, which would describe the new optional input for the plugin.

Any comments are welcome. Also, if you could tag this pr as a hacktoberfest pr, that would also be greatly appreciated! ;)

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@KalleOlaviNiemitalo
Copy link

Requested in JENKINS-66820.

@KalleOlaviNiemitalo
Copy link

If you add a help-abort.html file similar to help-id.html, I think its content will then be displayed as help in the snippet generator UI and copied to the online documentation.

@KalleOlaviNiemitalo
Copy link

help-ok.html seems to be missing as well. It would be nice to add that, too.

@gyula-s
Copy link
Author

gyula-s commented Oct 14, 2021

I've added both files. If you have any suggestions on how should they be worded, I welcome any suggestions.

@KalleOlaviNiemitalo
Copy link

The file name does not match -- help-aport.html should be help-abort.html.

The first and second paragraphs of help-ok.html have the same text.

AFAIK, HTML does not allow pre inside p.

Other than that, the wording looks OK to me. Have you tested that the pipeline snippet generator finds the help files and displays them correctly?

@gyula-s
Copy link
Author

gyula-s commented Oct 15, 2021

Fixed the filename.
Removed duplicate paragraph.
Moved pre outside of the p tag.
Note to self taken, to not to commit and push before bed. :)

I couldn't test the the pipeline snippet generator. Don't know where to look or how. If you could check that, it would be appreciated.

@k2glyph
Copy link

k2glyph commented Jun 23, 2022

any update here?

Copy link
Member

@rsandell rsandell left a comment

Choose a reason for hiding this comment

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

Unit test?

@rsandell
Copy link
Member

I just can't remember if there are any assumptions about abort being actually called that?

@jglick
Copy link
Member

jglick commented Nov 17, 2022

if there are any assumptions about abort being actually called that?

Not that I can think of.

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.

Looks OK.

gyula-s and others added 2 commits November 17, 2022 17:10
…s/input/InputStep/help-abort.html

Co-authored-by: Jesse Glick <[email protected]>
…s/input/InputStep/help-ok.html

Co-authored-by: Jesse Glick <[email protected]>
@gyula-s
Copy link
Author

gyula-s commented Nov 17, 2022

To reply to all the previous comments and questions and requirements for tests and all that...
I've no idea how to run this locally. I didn't know how to do that when I've opened this PR (13 months ago) and I definitely don't know it now.

Anyone wants this to be built... feel free to take over, because I'm not going to be able to put any more time in this.

@jglick
Copy link
Member

jglick commented Nov 17, 2022

I couldn't test the the pipeline snippet generator. Don't know where to look or how.

It is possible to write automated tests for this as well as the label usage, but probably overkill in this simple case, especially where overall sanity of the GUI is key. Assuming you have a JDK and Maven installed, just run

mvn hpi:run

open the provided link, create a project, Configure, click the link for Pipeline Syntax (opens in new tab), select input, play around with options generating sample Groovy, try copying the sample into the project and running it and approving vs. cancelling.

@gyula-s
Copy link
Author

gyula-s commented Nov 17, 2022

I couldn't test the the pipeline snippet generator. Don't know where to look or how.

It is possible to write automated tests for this as well as the label usage, but probably overkill in this simple case, especially where overall sanity of the GUI is key. Assuming you have a JDK and Maven installed, just run

mvn hpi:run

open the provided link, create a project, Configure, click the link for Pipeline Syntax (opens in new tab), select input, play around with options generating sample Groovy, try copying the sample into the project and running it and approving vs. cancelling.

Sorry... I just don't have the capacity to do this anymore.

@arglampedakis
Copy link

Any news about this? Does anyone understands why the check was not successful? Is there anything else that should be done for this to be merged?

@B34v0n
Copy link

B34v0n commented Mar 1, 2024

Any news on this?

@giannello giannello mentioned this pull request Jul 11, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants