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-1382] [CLOUD-1383] Add openshift.io/display-name to all image stream tag definitions #260

Merged
merged 3 commits into from
May 25, 2017

Conversation

kyguy
Copy link
Contributor

@kyguy kyguy commented Apr 24, 2017

…s to all image stream tag definitions

Some files have multiple image Stream Tag definitions, I added the openshift/display-name var to the image stream tag definitions which refer to the application being built from the template. For example, for templates like eap64-amq-persistent-s2i.json, I added the openshift/display-name vars to the image stream tag definitions which were named ${APPLICATION_NAME}:latest but not to the ones named "jboss-eap64-openshift:1.4" or "jboss-amq-62:1.3". Is this the right way to do this?

*NOTE: My script cleaned up some typos and indentation(a couple of the datavirt files had duplicate env lists). I can put this in a seperate commit/PR if you think that is better.

@rcernich
Copy link
Contributor

Hey @kyguy, sorry it took me so long to get to this. Wrt the extraneous changes, please remove them (or use OrderedDict in your script, assuming it's py). They pollute the diff's with unrelated changes.

Regarding the display names, these should apply to the items in jboss-image-streams.json, not in the templates.

@rcernich
Copy link
Contributor

Since you're going to be in there, feel free to address CLOUD-1382 at the same time.

@kyguy
Copy link
Contributor Author

kyguy commented May 17, 2017

Commits:

  • Cleaning up spacing and indentation
  • [ CLOUD-1382 ]: Add openshift.io/display-name to all image stream tag definitions
  • [ CLOUD-1383 ]: Add openshift.io/display-name and description annotations to all image stream tag definitions

-Updated the correct files, let me know if the labels look appropriate

  • I put the indentation and spacing fixes in a separate commit as part of this MR, is that proper protocol? Should I put the format changes in a separate MR? I have some ideas about future format handling below, let me know what you think:

I have been using a Python script with the json library to make changes across the template files.

In order to keep the diffs clean I have been using the following arguments when parsing and writing to the template files.
data = OrderedDict() ... json.dump(data, f, indent=4, separators=(',', ': '))

  • The script corrects any inconsistencies among the files(indentations, spacing). It seems that my script occasionally comes across these inconsistencies and auto corrects them. For the sake of keeping the templates clean, I was thinking we declare a format standard for the template json files. For instance, we could declare that a proper indentation is 4 spaces and that there be a single space after separators(such as colons). What do you think?

  • If you think this is a good idea, I can add a cleaning script that people can run before they commit their changes. By doing this, we can reduce the formatting commits and keep the formatting of the files consistent.

@kyguy kyguy changed the title WIP: [CLOUD-1383] Add openshift.io/display-name and description annotation… WIP: [CLOUD-1382] [CLOUD-1383] Add openshift.io/display-name and description annotation… May 17, 2017
@kyguy kyguy changed the title WIP: [CLOUD-1382] [CLOUD-1383] Add openshift.io/display-name and description annotation… WIP: [CLOUD-1382] [CLOUD-1383] Add openshift.io/display-name to all image stream tag definitions May 17, 2017
@kyguy
Copy link
Contributor Author

kyguy commented May 19, 2017

I mimicked the format of the OpenShift example image-streams. Although the OpenShift example image-streams have the "openshift.io/display-name" label in both the annotations and tags, they don't seem to appear in the latest version of the OpenShift web console for the image-streams. Maybe the "openshift.io/display-name" values (for the image-streams) are planned for a future release? Still investigating this though, I will let you know what I find

@kyguy kyguy changed the title WIP: [CLOUD-1382] [CLOUD-1383] Add openshift.io/display-name to all image stream tag definitions [CLOUD-1382] [CLOUD-1383] Add openshift.io/display-name to all image stream tag definitions May 22, 2017
@rcernich
Copy link
Contributor

Not sure what you mean. Looking at the Ruby image, the display-name is "Ruby" while the name is ruby. We can't change the name of the image streams, as this is what is used to reference the image stream in the rest of OpenShift. Looking at the console, it displays "Ruby" not "ruby."

It also appears that the description display is specific to the tag selected in the drop down.

@kyguy kyguy changed the title [CLOUD-1382] [CLOUD-1383] Add openshift.io/display-name to all image stream tag definitions WIP: [CLOUD-1382] [CLOUD-1383] Add openshift.io/display-name to all image stream tag definitions May 22, 2017
@kyguy
Copy link
Contributor Author

kyguy commented May 24, 2017

Updated the with metadata annotations, so now the openshift.io/display-names are appearing in the catalogue, I also updated the openshift.io/display-names to match the descriptions better

@kyguy kyguy changed the title WIP: [CLOUD-1382] [CLOUD-1383] Add openshift.io/display-name to all image stream tag definitions [CLOUD-1382] [CLOUD-1383] Add openshift.io/display-name to all image stream tag definitions May 24, 2017
@rcernich rcernich merged commit edba9c4 into jboss-openshift:master May 25, 2017
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

2 participants