-
Notifications
You must be signed in to change notification settings - Fork 350
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
add tags and descriptions to default runtime images #1023
Conversation
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
Looks like the
Will take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good Patrick - I like the choice of tags. I guess we should hold off on this merge until #985 is merged - correct? Or does the schema validation tolerate the additive properties?
Just had the one suggestion for the Makefile validation - thanks for catching that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Patrick.
I tested using the master branch, which didn't include the PR. Everything appeared to work as expected. Unless I am missing something this PR should be safe to merge. |
Have you changed the image tags? There are some images that don't have curl and/or python3 and that is a blocker for us. |
Also, I noticed we are adding an |
It is required. I think some of the descriptions Patrick chose to make different are fine - like using parenthesized values, etc. What is the reason for making them the same? |
No. The problem is that the existing parsing logic didn't properly extract the image name from the json file, resulting in a trailing comma: |
The description was there all along but we never specified any values, henceI created the issue.
But you are right in that the description is currently nowhere surfaced to the user outside of the editor. Why it isn't I don't know. |
I do think having a description is important to convey information that enables the user to make a well-informed decision which image to pick. Granted there is currently no additional information in the descriptions I've added but that doesn't mean my draft proposals cannot be tweaked to do that, if neccessary. |
Agree, which in this case I would expect that the description would provide more info than what is on the display name, but in some cases, we are providing less info. |
I can make the descriptions the same if that addresses your concern. |
This PR
Makefile
to address a parsing error that was triggered by above changesDeveloper's Certificate of Origin 1.1