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

Added copy to clipboard button to agent launch snippets #7625

Merged
merged 13 commits into from Feb 20, 2023

Conversation

prathi-mani
Copy link
Contributor

@prathi-mani prathi-mani commented Feb 4, 2023

See JENKINS-70503.

Screenshot

Testing done

  • Added copy of clipboard button "copyButton" for the agent in the "core/src/main/resources/hudson/slaves/JNLPLauncher/main.jelly" and tested this file and added that screenshot of the new design in comments sections of this PR.
  • Tested interactively with jenkins.war built from this pull request. Interactive testing requires an HTTPS connection because the browsers only allow the copy to clipboard over HTTPS

Proposed changelog entries

  • Add a copy button for the code snippets that start agents.

Proposed upgrade guidelines

N/A

Submitter checklist

  • The Jira issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • There is automated testing or an explanation as to why this change has no tests.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@prathi-mani
Copy link
Contributor Author

image

Please check the preview for the added copy to clipboard image format and let me know if anything has to be changed in the design

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Feb 4, 2023

Thanks for the pull request!

@prathi-mani please add a description of the testing that you've done under the Testing done heading. It is a required field before we will merge a pull request.

Could you also extend the title and the description so that the automated changelog will tell readers where the new copy button has been added?

Please add a proposed changelog entry that follows the guidance included in the pull request template.

Please remove the broken link to JENKINS-XXXXX since there does not appear to be a Jira ticket that should be associated with this pull request.

@MarkEWaite MarkEWaite added the needs-security-review Awaiting review by a security team member label Feb 4, 2023
@MarkEWaite MarkEWaite self-assigned this Feb 4, 2023
@StefanSpieker
Copy link
Contributor

I think it is JENKINS-70503

@prathi-mani prathi-mani changed the title Added Copy to clipboard Added Copy to clipboard button to copy code snippets in agents. Feb 5, 2023
@prathi-mani prathi-mani changed the title Added Copy to clipboard button to copy code snippets in agents. Added copy to clipboard button to copy code snippets in agents. Feb 5, 2023
@prathi-mani
Copy link
Contributor Author

Thanks @MarkEWaite for the comments, I am new to this open source contribution, I have added the descriptions to my best of ability.
Please let me know if anything has to be changed or added .

Thanks in advance for your guidance.

@MarkEWaite MarkEWaite added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Feb 5, 2023
@MarkEWaite
Copy link
Contributor

Thanks @MarkEWaite for the comments, I am new to this open source contribution, I have added the descriptions to my best of ability. Please let me know if anything has to be changed or added .

Thanks in advance for your guidance.

Thanks very much for the additions! I've made some minor adjustments (use imperative mood for the changelog entry, etc.).

@MarkEWaite MarkEWaite changed the title Added copy to clipboard button to copy code snippets in agents. Added copy to clipboard button to agent launch snippets Feb 5, 2023
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks very much for the pull request.

When I used the copy button as implemented in this pull request in my development environment without https, the text was not copied to the clipboard on either Firefox or Google Chrome, even from the other pages that implement it (like the script console page). I see the same behavior with my other Jenkins controllers that are not running over https. I hadn't realized that copy to clipboard is only allowed in secure contexts - over an HTTPS connection, not over an HTTP connection. Thanks for teaching me something very useful!

The text is copied in the script console test that I ran with https://weekly.ci.jenkins.io using the script console.

I'll need some more time to configure a controller running on https so that I can test this change. Thanks very much for teaching me something fundamental about the clipboard and HTTPS

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

It would probably make sense to move the copy button out of the <pre> block right next to the text above it.
This proposal aligns with our overall usage of copy buttons.

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Feb 6, 2023

I configured a copy of the jar file with the changes and found that the clipboard does not include the text for:

curl -sO https://home.markwaite.net/jenkins/jnlpJars/agent.jar

The other items on the page copy as expected to the clipboard.

There is extra text inserted into the page that mentions copy_.... That extra text needs to be removed before we merge this.

I like the suggestion from @NotMyFault that we should look to make the implementation consistent with other copy buttons.

@prathi-mani
Copy link
Contributor Author

Hi @MarkEWaite , I have removed the extra text from the file.

And as per my understanding the curl command to download the agent.jar and the java command to execute are two different, so I have inserted two copy buttons to differentiate them from one and another.

Please let me know if we need to make it a single copy button at the top of the code snippet.

@MarkEWaite
Copy link
Contributor

Thanks very much! I see that the text has been removed. I like the "Click to copy" at the end of each line. I think it makes sense

I'm able to copy the echo statement that creates secret-file and both the java -jar statements on my Windows computer running Google Chrome.

Unfortunately, when I try to copy the curl command, the icon indicates that the text has been copied, but the text is not copied. Can you investigate the difference between the copy associated with curl and the other three copies that worked?

@daniel-beck
Copy link
Member

It would probably make sense to move the copy button out of the <pre> block right next to the text above it.

There's multiple buttons in each case. Are you suggesting to change the behavior to only have one button per block?

@daniel-beck daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Feb 13, 2023
@jglick
Copy link
Member

jglick commented Feb 13, 2023

copy to clipboard is only allowed […] over HTTPS

Hmm, that may explain #6698 (comment)

@MarkEWaite
Copy link
Contributor

Hmm, that may explain #6698 (comment)

I wonder if it would be better for users if we made the copy button visible only if we're running over a secure connection. Unfortunately, I don't know how to detect if the connection between the browser and the controller is secure.

Creates a heading over the region to be copied and create a set of
instructions for Unix agents with a different set of instructions
for Windows.  The `curl.exe` call must be different on Windows due to
PowerShell having a `curl` cmdlet that is not compatible with command line
`curl.exe`.

Passed my interactive testing with a session in a secure connection.
@MarkEWaite
Copy link
Contributor

MarkEWaite commented Feb 18, 2023

It would probably make sense to move the copy button out of the \<pre\> block right next to the text above it.

There's multiple buttons in each case. Are you suggesting to change the behavior to only have one button per block?

Switched to one button per region in 2b634cd . The implementation now looks very similar to the copy button that is available from the script console after a command has run in the console.

Looks like this

screencapture-rhel-8-a-markwaite-net-8080-jenkins-computer-inbound-agent-2023-02-17-17_34_17

@MarkEWaite MarkEWaite added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Feb 19, 2023
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

The changes as they are now have passed my interactive testing. I believe they are ready for review by others. They've passed the CI job previously and are expected to pass again.

Once we have a second approval, I believe we are ready to start the countdown clock for the merge.

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Thanks!

@MarkEWaite
Copy link
Contributor

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

@MarkEWaite MarkEWaite merged commit a6ff3af into jenkinsci:master Feb 20, 2023
@jglick
Copy link
Member

jglick commented Feb 21, 2023

I don't know how to detect if the connection between the browser and the controller is secure.

On the server side it would be difficult; you would need to check X-Forwarded-Scheme etc. I think it could work for the client side to automatically check the window.location protocol and grey out the copy button or something.

(Unlikely to affect many real users, just a point of confusion when testing locally.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues squash-merge-me Unclean or useless commit history, should be merged only with squash-merge
Projects
None yet
6 participants