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

Prepare for prototype removal #491

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

rsandell
Copy link
Member

@rsandell rsandell commented Oct 23, 2023

See https://www.jenkins.io/blog/2023/05/12/removing-prototype-from-jenkins/

Testing done

Manual testing done:

  • Created a domain and added credentials to it
  • Created a new credential from the "Add" button on a Git SCM configuration page.

Submitter checklist

Edit tasklist title
Beta Give feedback Tasklist Submitter checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
    Options
  2. Ensure that the pull request title represents the desired changelog entry
    Options
  3. Please describe what you did
    Options
  4. Link to relevant issues in GitHub or Jira
    Options
  5. Link to relevant pull requests, esp. upstream and downstream changes
    Options
  6. Ensure you have provided tests - that demonstrates feature works or fixes the issue
    Options

@rsandell rsandell requested a review from a team as a code owner October 23, 2023 15:47
@rsandell
Copy link
Member Author

rsandell commented Oct 23, 2023

This will merge conflict with #481 fixed.

Copy link
Contributor

@raul-arabaolaza raul-arabaolaza left a comment

Choose a reason for hiding this comment

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

I have confirmed the issues by trying to add a new domain, there is a very clear error message on my js console about $ that does not happen when you use a jenkins version that still contains prototype.

The concrete fixes look good to me

@basil basil removed their request for review October 24, 2023 17:39
@ampuscas
Copy link

I have tested it:
In a Jenkins which has prototype removed, there are errors:
Screenshot

When I start Jenkins and take into account this PR, there are no errors, and I can create a new domain and add credentials, without any errors

@rsandell
Copy link
Member Author

I've reverted the changes to select.js so now creating a credential from for example a Git SCM credentials input on a job configuration page works again.

Copy link
Contributor

@yaroslavafenkin yaroslavafenkin left a comment

Choose a reason for hiding this comment

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

Looks fine security wise

@raul-arabaolaza raul-arabaolaza merged commit 5ec13ee into jenkinsci:master Oct 26, 2023
15 checks passed
@car-roll car-roll added developer and removed chore labels Oct 26, 2023
@Wadeck
Copy link
Contributor

Wadeck commented Nov 28, 2023

Potentially introduced JENKINS-72364

@@ -3,5 +3,5 @@ Behaviour.specify("#credentials-add-submit", 'credentials-dialog-add', 0, functi
});

Behaviour.specify("#credentials-add-abort", 'credentials-dialog-abort', 0, function (e) {
e.onclick = (_) => window.credentials.dialog.hide();
e.onclick = (_) => window.credentials.dialog.style.display = "none";
Copy link
Member

Choose a reason for hiding this comment

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

This change appears to be in error, as this was YUI code, not Prototype code.

MarkEWaite added a commit to MarkEWaite/credentials-plugin that referenced this pull request Dec 7, 2023
One change from jenkinsci#491
was applied to a reference that was not using Prototype.js.

https://issues.jenkins.io/browse/JENKINS-72364 is the issue report.

Confirmed interactively that I can see the problem before the change
and that with this change the problem is resolved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants