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

[FIXED JENKINS-44204] Provide essential views for clouds #2887

Merged
merged 2 commits into from May 29, 2017

Conversation

olivergondza
Copy link
Member

@olivergondza olivergondza commented May 16, 2017

Description

Provide essential views for clouds

Cloud pages can now be customized by actions and sidepanel, that can be further customized by widgets.

  • Clouds now have default index view so they can be linked.
  • Cloud is actionable now.
  • Clouds can present relevant info via main or top view instead of overriding the index page as a whole.
  • Cloud is now actionable and actions can contribute snippets for main portion of index view, sidepanel's task list or list of boxes underneath.

cloud-stats plugin integration in downstream PR: jenkinsci/cloud-stats-plugin#3
cloud-stats

See JENKINS-44204.

Changelog entries

Views for Jenkins clouds.

Submitter checklist

  • JIRA issue is well described
  • Link to JIRA ticket in description, if appropriate
  • Appropriate autotests or explanation to why this change has no tests
  • For new API and extension points: Link to the reference implementation in open-source (or example in Javadoc)

Desired reviewers

@jenkinsci/code-reviewers, this is a new view API for cloud plugins.

Cloud pages can now be customized by actions and sidepanel, that can be
further customized by widgets.
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 All comments are rather improvement proposal excepting the missing documentation to Cloud Javadoc. I would follow the approach in other such API classes when new pages are documented

* @since TODO
* @return Jenkins relative URL.
*/
public @Nonnull String getUrl() { return "cloud/" + name; }
Copy link
Member

Choose a reason for hiding this comment

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

I would not inline the code here. IIRC Oracle codestyle does not advertise it

*/
public @Nonnull String getUrl() { return "cloud/" + name; }

public @Nonnull String getSearchUrl() {
Copy link
Member

Choose a reason for hiding this comment

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

Add Javadoc as well? Just to explain difference. Maybe it is also a subject for default implementation like @jglick did for job APIs

<l:layout title="${it.name}">
<st:include page="sidepanel.jelly" it="${it}"/>
<l:main-panel>
<h1>Cloud ${it.name}</h1>
Copy link
Member

Choose a reason for hiding this comment

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

"Cloud" should be localizable IMHO

<st:include page="summary.jelly" from="${action}" optional="true" it="${action}" />
</j:forEach>

<st:include page="main.jelly" optional="true"/>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe makes sense to print something if there is no such main page. NIT

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is Jenkins knows very little about the clouds itself.

@@ -81,7 +83,7 @@
* @see NodeProvisioner
* @see AbstractCloudImpl
*/
public abstract class Cloud extends AbstractModelObject implements ExtensionPoint, Describable<Cloud>, AccessControlled {
public abstract class Cloud extends Actionable implements ExtensionPoint, Describable<Cloud>, AccessControlled {
Copy link
Member

Choose a reason for hiding this comment

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

Cloud Javadoc needs to describe the new UI components

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean the ones I introduced, no those that Actionable brought in, right?

Copy link
Member

Choose a reason for hiding this comment

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

yep. The ones introduced by Actionable are not mandatory though it would not hurt

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

👍

@olivergondza olivergondza removed the request for review from daniel-beck May 19, 2017 12:12
@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label May 19, 2017
@oleg-nenashev
Copy link
Member

As @olivergondza said in IRC, he prefers to wait for a while for feedback from @jenkinsci/code-reviewers . So I will put this PR on-hold & let Oliver to decide when to merge

@oleg-nenashev oleg-nenashev added on-hold This pull request depends on another event/release, and it cannot be merged right now ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels May 20, 2017
@daniel-beck
Copy link
Member

@olivergondza Ping?

@olivergondza
Copy link
Member Author

No more feedback so I will go on and merge.

@olivergondza olivergondza merged commit dbd645a into jenkinsci:master May 29, 2017
olivergondza added a commit that referenced this pull request May 29, 2017
[FIXED JENKINS-44204] Provide essential views for clouds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-more-reviews Complex change, which would benefit from more eyes on-hold This pull request depends on another event/release, and it cannot be merged right now ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
3 participants