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

node / node-sass / CircleCI updates & fixes #418

Merged

Conversation

virtuoushub
Copy link

@virtuoushub virtuoushub commented May 10, 2020

Closes:

Description
Allows for building of project in Node 10, 12, 13, and 14.
Does builds (and more) for each runtime in CircleCI.
See: https://circleci.com/blog/circleci-matrix-jobs/

Compatibility
Improves compatibility with Node 13 and 14.

edit: Looks like this CircleCI change will not work with this repo's current checks in GitHub as they need to be explicitly "ci/circleci: build" and "ci/circleci: lint". The matrix jobs now make one build/lint per Node runtime (e.g. "ci/circleci: build-10.20.1", "ci/circleci: lint-10.20.1" ), so that would need to be changed.

Caveats
The CircleCI config could probably be cleaned up.

@guastallaigor guastallaigor requested a review from a team May 10, 2020 03:12
@guastallaigor guastallaigor added bug Something isn't working waiting - reviewer Waiting for the reviewer to address some situations labels May 10, 2020
@doliG
Copy link

doliG commented May 11, 2020

Very nice one, thank you 👍

trezy
trezy previously approved these changes May 21, 2020
Copy link
Member

@trezy trezy left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@guastallaigor
Copy link
Member

Not sure what do to here, looks like the builds are stucked, and because of that I can't merge 😢

@virtuoushub
Copy link
Author

@guastallaigor see my comment

Looks like this CircleCI change will not work with this repo's current checks in GitHub as they need to be explicitly "ci/circleci: build" and "ci/circleci: lint".

I'd imagine changing some configuration on this repo's checks will fix it, see also: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-status-checks

@guastallaigor
Copy link
Member

@virtuoushub I see, I've tried to change inside the Circle CI settings, but unfortunately I don't have access to it, and I didn't find it anywhere else as of this moment.

@virtuoushub
Copy link
Author

virtuoushub commented May 29, 2020

@guastallaigor if you are an admin of this repository this should be a direct link to where you can turn on/off checks: https://github.com/nostalgic-css/NES.css/settings/branch_protection_rules/new if that link didn't work here is what GitHub's GUI looks like, which should be similar for this repo:
image

If you are not an admin, we will need one to help us; or I will need to refactor this PR to somehow have those checks as well.

@BcRikko / @trezy are you able to assist?

@guastallaigor
Copy link
Member

@virtuoushub I'm not a admin of this repository, for me the Settings tab do not exist.

I think it's too much checks to run for every PR, if possible it's better if we found a way to decrease them.

@virtuoushub
Copy link
Author

virtuoushub commented May 29, 2020

@guastallaigor while I agree the CircleCI config can be cleaned up, part of the point of this PR was to prove that the project can be built/used in multiple versions of Node. A lot of the checks happen in parallel so I do not think they are actually too many.

Having the CI build with various versions of node might help prevent future issues like:

Of the multiple checks per node version currently happening, I think the only one that does not need to happen in every version of node is the lint. I will work on making that one happen only once.

@virtuoushub virtuoushub force-pushed the node-sass-circle-ci branch 2 times, most recently from 054ab44 to d78e7b3 Compare May 29, 2020 19:31
@guastallaigor
Copy link
Member

I understand, regardless I can only merge once all the checks that we have right now were successful.

You don't need to go out of your way to make this work, we can also wait for the other members that have admin access to the repo to remove the checks, but it can take a while.

@virtuoushub virtuoushub force-pushed the node-sass-circle-ci branch 6 times, most recently from 48d04cc to f52cf9a Compare May 30, 2020 18:19
Closes nostalgic-css#414
Closes nostalgic-css#415 (that PR willed be superseded b/c this one upgrades node-sass to allow for Node 14)
@virtuoushub virtuoushub force-pushed the node-sass-circle-ci branch 2 times, most recently from 1fad0e4 to 18ce3da Compare June 1, 2020 22:20
@virtuoushub virtuoushub force-pushed the node-sass-circle-ci branch 3 times, most recently from 9e1e583 to 22ff281 Compare June 2, 2020 14:15
Copy link
Member

@guastallaigor guastallaigor left a comment

Choose a reason for hiding this comment

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

I don't know much about Circle CI configuration stuff, but I see that all the checks were a success, and looked into them inside Circle CI.
Also this PR is going to fix the node-sass problem as well.
So LGTM 👍

@guastallaigor guastallaigor merged commit 58e1da8 into nostalgic-css:develop Jun 2, 2020
@guastallaigor
Copy link
Member

Thank you so much @virtuoushub and @SecretBase for your hard work 🎉
I couldn't have it done better myself for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants