-
Notifications
You must be signed in to change notification settings - Fork 346
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
chore(deps): update win orb to v2 #2221
Conversation
Based on a quick look to the previous and last version of the circli ci windows orb:
It looks that the Before merging this PR we will have to update the circle ci config accordingly. |
71315eb
to
7d31b98
Compare
7d31b98
to
b2ad5bc
Compare
…he circleci/windows@2.4.0 orb
TODO:
Update (2021/06/07):
Initial analysis (2021/06/01): Unfortunately the changes described in #2221 (comment) are enough to make fix the circleci config, but that is revealing a odd issue with the test.lib.imports.js test, which is currently failing with the following error message:
|
Codecov Report
@@ Coverage Diff @@
## master #2221 +/- ##
=======================================
Coverage 99.88% 99.88%
=======================================
Files 32 32
Lines 1699 1699
=======================================
Hits 1697 1697
Misses 2 2 Continue to review full report at Codecov.
|
@Rob--W I was investigating this issue right before going on PTO last week, but after fixing the original issue I did trigger another one. I took a more deeper look today and updated this PR with a fix (the issue and fix are described in #2221 (comment)). I plan to merge this soon, do you want to take a quick look first? (mainly in terms of getting a second opinion about the readability and maintainability of the fix applied to the circleci config file, there isn't much to review in this PR besides that). |
.circleci/config.yml
Outdated
## NOTE: the nodejs version shipped with circleci/windows@2.4.0 orb | ||
## does fail on the test-functional test related to importing | ||
## web-ext as a library, the issue seems to have been fixed on more | ||
## recent nodejs versions. |
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.
Can you be more explicit about the broken/fixed versions? And add the explicit sentence "Install a recent version without this issue." (Change "recent" to something more accurate if needed)
And if you think that it helps, also include a link to this PR/comment.
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.
I've been a bit lazy, and so I didn't run a bisect, but in the end it would be nice to write down what was the broken range while I'm still around this issue.
Based on a quick bisect, the range between 12.11.1 and 12.15.0 (included) is broken, the first version to work as expected is v12.16.0.
I updated the comment accordingly, but I didn't looked for the first version broken, I don't think we care too much about that in the end.
… shipped on the circleci/windows@2.4.0 orb
This PR contains the following updates:
1.1.0
->2.4.0
Configuration
📅 Schedule: At any time (no schedule defined).
🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.
♻️ Rebasing: Renovate will not automatically rebase this PR, because other commits have been found.
🔕 Ignore: Close this PR and you won't be reminded about this update again.
This PR has been generated by WhiteSource Renovate. View repository job log here.