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

Remove the WorkflowRun/index.jelly file #364

Merged
merged 9 commits into from
Nov 8, 2024

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Jun 9, 2023

Relates to jenkinsci/jenkins#8110

This PR removes the WorkflowRun/index.jelly file as if #8110 is merged, it'd be possible to rely on the Run/index.jelly file from Jenkins instead.

It would be preferable to have plugins extend pages rather than to reimplement them to make changes in core easier and more consistent. This will be important with upcoming works on the Jenkins user experience, such as introducing app bars, new widgets and adjusting the sidebar.

If there's anything I've missed do let me know.

Before
image

After
image

Testing done

Submitter checklist

  • 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

@timja
Copy link
Member

timja commented Jun 9, 2023

What is that failed to determine thing?

@jglick jglick marked this pull request as draft June 9, 2023 19:41
@jglick
Copy link
Member

jglick commented Jun 9, 2023

Marking as draft since this cannot be merged as is—only if and when the core PR is merged, and the baseline version here is updated accordingly.

@jglick
Copy link
Member

jglick commented Jun 9, 2023

What is that failed to determine thing?

Something about changesets. jenkinsci/jenkins#8110 (comment)

@jglick
Copy link
Member

jglick commented Jun 9, 2023

Set the jenkins.version here to an incremental build of the specific revision of the core PR it matches.

@jglick
Copy link
Member

jglick commented Jun 12, 2023

Does this still need work to adapt to jenkinsci/jenkins@f405797?

pom.xml Outdated Show resolved Hide resolved
@jglick
Copy link
Member

jglick commented Apr 12, 2024

@janfaracik can you please check whether this is still working as expected, and if so, mark it ready for review?

@janfaracik
Copy link
Contributor Author

Before
Screenshot 2024-05-17 at 11 19 13

After
Screenshot 2024-05-17 at 11 17 49

Tried it out with a basic pipeline, looks to be working fine. Only one issue has been raised for the core PR and that's https://issues.jenkins.io/browse/JENKINS-73168

@jglick
Copy link
Member

jglick commented May 17, 2024

OK, just mark ready for review whenever you think it is safe to merge.

@janfaracik janfaracik marked this pull request as ready for review May 18, 2024 09:00
@jglick jglick requested review from daniel-beck, NotMyFault, timja and a team November 7, 2024 20:14
@jglick
Copy link
Member

jglick commented Nov 7, 2024

(Requesting review from reviewers of the core PR, since they probably more context for this than I.)

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM, I tested this at the time

@jglick jglick merged commit b05b364 into jenkinsci:master Nov 8, 2024
17 checks passed
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.

3 participants