Skip to content

Conversation

@amisevsk
Copy link
Contributor

What does this PR do?

Replaces mikefarah/yq with kislyuk/yq and reworks scripts to accomodate this change:

  • Instead of manually checking meta.yamls for required fields, uses jsonschema to validate against a schema file
    • This also removes the requirement for the check_plugins_images.sh script
    • This process is quite slow, taking around 25 seconds for all our current metas
  • The index.sh script is greatly simplified due to the capabilities of yq
  • There are a few workarounds in place because invoking yq in a loop is tremendously slow
    • To get plugin ids for reporting purposes (i.e. in util.sh), I'm using grep instead of yq (replacing this function with yq adds ~30 seconds to the build time). This could fail if any of the fields are not on the same line
  • There are also a couple workarounds since yq doesn't support in-place editing:
    • To set latestUpdateDate, we use sed to append a line to each file.
    • I'm not clear on what the latestUpdateDate field is for, since it's discarded by the plugin broker

Dockerfile.rhel Outdated
Copy link
Contributor

@nickboldt nickboldt Aug 20, 2019

Choose a reason for hiding this comment

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

Do we use this file? Why not delete it? Maybe we rename Dockerfile to Dockerfile.alpine, and rename Dockerfile.rhel to Dockerfile so it's the default one used? (Yes, this can be done in another commit.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll create an issue for moving to ubi8, I don't think alpine is necessary, moreso convenient.

The only concern would be due to the difference in image size, since the broker has to be pulled for every workspace start, but I wonder how much of an issue this would actually be considering it would probably be cached pretty quickly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nevermind I see you've beat me to the punch: eclipse-che/che#14113

I'd do it as a separate PR as I don't think it's a simple drop-in replacement.

@amisevsk
Copy link
Contributor Author

Rebased on master, fixed the shellcheck issue (can't put anything after a directive it seems), and removed an errant paste in the schema.

@amisevsk
Copy link
Contributor Author

Relaxed the validation requirement for version numbers to include "number", since the version for che-incubator/cpptools/0.1 parses as a number and not a string (not an issue for the other plugins, as e.g. 7.0.0 cannot be a number).

@amisevsk
Copy link
Contributor Author

Rebased on master to resolve merge conflict.

Instead of using the golang-based version of yq, use the Python jq
wrapper, also named yq.

Also updates plugin schema validation to use jsonschema, and adds a
meta.yaml.schema file.

Signed-off-by: Angel Misevski <[email protected]>
@nickboldt
Copy link
Contributor

nickboldt commented Sep 4, 2019

+1 for this solution. Make it sew, number one.

https://1188277.v1.pressablecdn.com/wp-content/uploads/2018/08/Make-it-Sew.jpg

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.

3 participants