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

Minor UX changes #173

Merged
merged 3 commits into from
May 2, 2024
Merged

Minor UX changes #173

merged 3 commits into from
May 2, 2024

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Jan 15, 2020

No description provided.

`exec` is not a real thing. There's `executable`, there's `exe`, but neither add value here.
@@ -25,7 +25,7 @@
<j:set var="APIInstance" value="${it.instance}" />
<j:set var="cloud" value="${instance.cloud}" />

<h3>This is a read-only view. Click <a href="${rootURL}/configure"> here</a> to configure your instances.</h3>
<h3>This is a read-only view. Click <a href="${rootURL}/configureClouds/"> here</a> to configure your instances.</h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

When testing this locally with mvn hpi:run, this didn't work. This is meant to point at the configure system page, which should be at "/configure"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daniel-beck changed this as of https://github.com/jenkinsci/jenkins/commits/jenkins-2.205 by jenkinsci/jenkins#4339 especially jenkinsci/jenkins@77f1242

It looks like LTS is currently 2.204.1 which means it just missed.

I guess once the next LTS arrives, I can include a change to the pom to bump the jenkins version which should result in your testing working.

<jenkins.version>2.138.4</jenkins.version>

Alternatively, I suppose w/ some amount of magic, I could ask jenkins if an endpoint exists and conditionally return the appropriate url.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW there's a placeholder in the existing configure page, so it's not like this is now wrong, just adds a level of indirection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From a UX perspective, getting a link to a page which just says you shouldn't be here is fairly insulting. That said, if the code path is inconsistent at this point, then either a branch in the code or a version bump at the next LTS seems warranted.

Copy link
Member

Choose a reason for hiding this comment

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

jenkins version is currently set to 2.401.3 so this has long since past :)

Copy link
Member

Choose a reason for hiding this comment

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

This URL has also been subsequently changed jenkinsci/jenkins#7658 in Jenkins version jenkins-2.403 to be manage/cloud/

Allthough there is a HTTP redirect in place for this.

Copy link
Member

Choose a reason for hiding this comment

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

@jtnord jtnord added the enhancement New feature or request label May 2, 2024
@@ -25,7 +25,7 @@
<j:set var="APIInstance" value="${it.instance}" />
<j:set var="cloud" value="${instance.cloud}" />

<h3>This is a read-only view. Click <a href="${rootURL}/configure"> here</a> to configure your instances.</h3>
<h3>This is a read-only view. Click <a href="${rootURL}/configureClouds/"> here</a> to configure your instances.</h3>
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
<h3>This is a read-only view. Click <a href="${rootURL}/configureClouds/"> here</a> to configure your instances.</h3>
<!-- TODO change the URL to ${rootURL}/manage/cloud/ when Jenkins version is 2.403 or higher -->
<h3>This is a read-only view. Click <a href="${rootURL}/configureClouds/"> here</a> to configure your instances.</h3>

@jtnord jtnord closed this May 2, 2024
@jtnord jtnord reopened this May 2, 2024
@jtnord jtnord merged commit d042ea2 into jenkinsci:develop May 2, 2024
20 of 22 checks passed
jtnord added a commit to jtnord/google-compute-engine-plugin that referenced this pull request May 2, 2024
amends jenkinsci#173 due to jenkinsci/jenkins#7658

bumps the baseline to avoid the redirect or adding a TODO
@jsoref jsoref deleted the ux branch May 2, 2024 15:53
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