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

[CLOUD-1384] Add openshift.io/display-name and description annota… #244

Closed
wants to merge 1 commit into from
Closed

Conversation

kyguy
Copy link
Contributor

@kyguy kyguy commented Feb 22, 2017

…tions to all templates

openshift.io/discription is copied from the description annotation. The script I ran cleaned up some of the spacing inconsistencies too(Is that okay?). Let me know what you think and what needs to be changed

@jwendell
Copy link
Member

I think "openshift.io/description" is redundant. Openshift already takes the "description" field into consideration (tested in web console for 3.3 and 3.4).

Also, IMO, the display-name could be more elaborated WRT abbreviations. For example:
A-MQ62 (Basic) → A-MQ 6.2 (Basic) or Red Hat JBoss A-MQ 6.2 (Basic)
Datagrid65 (MySQL) → Data Grid 6.5 (Using MySQL) or Red Hat JBoss DataGrid 6.5 (Using MySQL)
...

@rcernich
Copy link
Contributor

Similar to @jwendell's comments. We should be using the product names (e.g. Red Hat JBoss A-MQ 6.2) and the description field is just "description". The templates in the origin repository have some additional annotations (not yet released), which include template.openshift.io/long-description. Also, CLOUD-1384 only refers to the display-name field. All of the annotation changes required are described in CLOUD-1381 and openshift.io/description (or template.openshift.io/long-description) is not on that list. I appreciate your enthusiasm though.

@kyguy
Copy link
Contributor Author

kyguy commented Feb 23, 2017

Thanks for the feedback, I have updated the files based on the comments. Let me know how it looks and if anything else needs to be changed

@kyguy
Copy link
Contributor Author

kyguy commented Feb 23, 2017

Wait hold one second on that I forgot one thing....adding now

@kyguy
Copy link
Contributor Author

kyguy commented Feb 23, 2017

Alright, changes made, now it is ready for your eyes again

@@ -6,7 +6,8 @@
"description": "Application template for JBoss A-MQ brokers. These can be deployed as standalone or in a mesh. This template doesn't feature SSL support.",
"iconClass": "icon-jboss",
"tags": "messaging,amq,jboss,xpaas",
"version": "1.3.1"
"version": "1.3.1",
"openshift.io/display-name": "Red Hat JBoss A-MQ 62 (Basic)"
Copy link
Member

Choose a reason for hiding this comment

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

Versions should have the dot. 62 → 6.2
This applies to all other changes.

@@ -3,7 +3,8 @@
"apiVersion": "v1",
"metadata": {
"annotations": {
"description": "Examples that can be installed into your project to allow you to test the A-MQ template. You should replace the contents with data that is more appropriate for your deployment."
"description": "Examples that can be installed into your project to allow you to test the A-MQ template. You should replace the contents with data that is more appropriate for your deployment.",
"openshift.io/display-name": "Red Hat JBoss A-MQ (App Secret)"
Copy link
Member

@jwendell jwendell Feb 23, 2017

Choose a reason for hiding this comment

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

Secrets don't need this. Moreover, these secrets are only for convenience purposes. They aren't used by templates. This applies to all secrets below.

"iconClass": "icon-jboss",
"tags": "sso,keycloak,java,jboss,xpaas",
"version": "1.3.2",
"openshift.io/display-name": "Red Hat JBoss SSO70 (Using HTTPS)"
Copy link
Member

@jwendell jwendell Feb 23, 2017

Choose a reason for hiding this comment

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

SSO70 → SSO 7.0 (Space and dot, applies to others below)

@kyguy
Copy link
Contributor Author

kyguy commented Feb 23, 2017

Dang, you have sharp eyes, I can't believe I missed those!

@kyguy kyguy changed the title WIP [CLOUD-1384] Add openshift.io/display-name and description annota… [CLOUD-1384] Add openshift.io/display-name and description annota… Mar 6, 2017
@jwendell
Copy link
Member

jwendell commented Mar 6, 2017

LGTM, except maybe, for this minor phrasing:
Product (Using <Database> Persistent) → Product (Using Persistent <Database>)

(here the database is mounted upon a persistent volume, so, we can say the database is persistent; or that we are using a persistent database)

You native English spoken guys may have a better feeling than I though.

@kyguy
Copy link
Contributor Author

kyguy commented Mar 6, 2017

I agree that this should be changed, what if we did something like:
Product (Using Persistent) → Product (Using With Persistent Storage)

Would that be ok too?

This way we would also clarify that products without a database have a persistent volume:
Product (Persistent) → Product (With Persistent Storage)

That is, if users are not sure what "(Persistent)" means. If they already know, this option could make things more verbose and confusing. Maybe I am over thinking this. Let me know what you think. I can change it easily either way.

@rcernich
Copy link
Contributor

rcernich commented Mar 6, 2017

It's difficult to say what's perfect. I think we should maybe use the following strategy:
Red Hat JBoss EAP 6.4 (Basic, no https) - the basic templates
Red Hat JBoss EAP 6.4 - the complete templates (i.e. https)
Red Hat JBoss EAP 6.4 (plus MySQL persistent)
Red Hat JBoss EAP 6.4 (plus MySQL ephemeral)

Something like that. We should get clarification on the exact name to use for EAP and should follow the scheme used by the other templates (e.g. the MySQL template). By separating out "basic" instead of https, we highlight that basic is special, since all the other templates support https.

@rcernich
Copy link
Contributor

Tweaked the names a bit, rebased and pushed.

@rcernich rcernich closed this Mar 16, 2017
@kyguy kyguy deleted the CLOUD-1384 branch April 24, 2017 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants