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

MDLSITE-5739 mustache_lint: error when running on openjdk11 #198

Merged
merged 8 commits into from
Jul 27, 2019

Conversation

kabalin
Copy link
Member

@kabalin kabalin commented May 1, 2019

More details:

As I can see, repo does not include package-lock.json, I guess this is intentional, as previously requirement was "^17.3.0" (any compatible version)? I do not use "^18.11.5" as it seems bring up unstable releases above that version when npm is initialised or updated. Perhaps we need to introduce package-lock.json to make sure that end user is using the same version which has been integration-tested here?

@kabalin kabalin force-pushed the MDLSITE-5739-master branch 2 times, most recently from 80de1a3 to b710385 Compare May 1, 2019 14:42
@kabalin
Copy link
Member Author

kabalin commented May 1, 2019

I think travis fails on https://github.com/moodlehq/moodle-local_ci/blob/master/mustache_lint/mustache_lint.sh#L30 (e.g. "11.0.2" > "1.8" 😉 ), will see tomorrow if it is possbile to do proper semver comparison.

@kabalin kabalin force-pushed the MDLSITE-5739-master branch from 843ac56 to 2ae4e56 Compare May 2, 2019 12:19
Copy link
Member

@stronk7 stronk7 left a comment

Choose a reason for hiding this comment

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

Thanks Ruslan, nice stuff. And great to have it covered and working against more envs! Let's see the remaining little details...

mustache_lint/mustache_lint.sh Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
How to upgrade vnu-jar:
1. Identify latest stable release at https://www.npmjs.com/package/vnu-jar
2. Amend version at package.json
3. Run npm upgrade
4. Commit changes.
@kabalin
Copy link
Member Author

kabalin commented May 28, 2019

Thinking more. May be we should group all mustache tests and run them in all available java environments, and not differentiate between trusty and xenial?

@stronk7
Copy link
Member

stronk7 commented Jun 13, 2019

Thanks @kabalin !

Just a couple of questions:

  1. With xenial becoming the default one... maybe we'll need to also add explicitly the trusty runs to the matrix/include? If I understand it properly we may end running xenial twice unless we explicitly set the dist for each run, isn't it?

  2. About testing on all available java environments I really don't know if it's worth it* . Surely I'd wait somebody to report that it's not working properly with java xxxxx instead of continuously try to keep the tests on top of a bunch of flavors and versions.

* note that, personally, i'm not sure, either, if it's worth testing it over multiple ubuntu distributions, it just doubles the running time when, by default, I'd expect things to continue working the same. But I don't oppose, just 1) above called my attention.

Other than that 1), I'm happy with this one as is, tell me that do you think so we can advance it...

Ciao :-)

@stronk7
Copy link
Member

stronk7 commented Jul 22, 2019

Any thought about that 1) above, @kabalin ? I had this completely forgotten, sorry, just arrived here today by coincidence (another PR).

@kabalin
Copy link
Member Author

kabalin commented Jul 25, 2019

Hi @stronk7, sorry I missed the your first comment completely (I was on holidays most of June).

  1. you are right :)
  2. I actually was thinking thought the same before reading "*" part. We know it works fine with openjdk8, so testing explicitly if it "still works" with old version probably not required. The default recommendation for anyone who has issues in older distro would be just to switch to xenial, which we tested against in this CI scenario. To the best of my knowledge, moodle-plugin-ci that is using this package as dependency has no critical issues using xenial either, apart of mustache linting, which this fix is addressing.

Changes will follow in a moment.

UPD: for some reason commit I added is shown above this comment.

@stronk7 stronk7 merged commit 99fec69 into moodlehq:master Jul 27, 2019
@stronk7
Copy link
Member

stronk7 commented Jul 27, 2019

Here we go, thanks!

@kabalin kabalin deleted the MDLSITE-5739-master branch July 27, 2019 09:09
@kabalin
Copy link
Member Author

kabalin commented Jul 27, 2019

👍

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.

2 participants