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

Make use of ci-info for detecting CI environments #8110

Closed
wants to merge 1 commit into from

Conversation

gowthamrm
Copy link
Contributor

Fixed #8093

Replaced the usage of process.env.CI and process.env.TRAVIS with require('ci-info').isCI

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for working on this!!

blueprints/app/files/config/targets.js Outdated Show resolved Hide resolved
blueprints/app/files/testem.js Outdated Show resolved Hide resolved
blueprints/module-unification-app/files/testem.js Outdated Show resolved Hide resolved
lib/models/project.js Outdated Show resolved Hide resolved
}));

afterEach(function() {
if (ORIGINAL_PROCESS_ENV_CI === undefined) {
delete process.env.CI;
delete ci.isCI;
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think we ever want to delete here. I’m also unsure how these tests can reliably pass (because now we don’t consistently control the CI state).

tests/fixtures/module-unification-addon/testem.js Outdated Show resolved Hide resolved
tests/fixtures/smoke-tests/js-testem-config/testem.js Outdated Show resolved Hide resolved
tests/runner.js Show resolved Hide resolved
@gowthamrm gowthamrm force-pushed the implement_ci_info branch 2 times, most recently from 06f1c3c to f14c43d Compare October 11, 2018 12:09
@gowthamrm
Copy link
Contributor Author

@rwjblue @kellyselden Thank you for your review comments. I have made the suggested changes and updated it here. Please have a look and let know if it is good to go.

Copy link
Member

@kellyselden kellyselden left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you!

@gowthamrm
Copy link
Contributor Author

@rwjblue Can you please check on this? Is there anything else need to be done?

@@ -1,12 +1,12 @@
'use strict';

const { isCI } = require('ci-info');
Copy link
Member

Choose a reason for hiding this comment

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

if we wanted to do this we would need to add an explicit ci-info dependency to every app 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Ah good catch.

Copy link
Member

Choose a reason for hiding this comment

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

as an alternative we could reexport the isCI variable ofci-info from ember-cli. might be the simpler alternative 🤔

@Turbo87
Copy link
Member

Turbo87 commented Feb 27, 2019

@gowthamrm do you have time to work on this again?

@rwjblue
Copy link
Member

rwjblue commented Oct 4, 2021

I'm going to close this for now (rebasing will be difficult), but I am absolutely still interested in migrating to usage of ci-info for any case we're currently using process.env.CI...

@rwjblue rwjblue closed this Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leverage ci-info for detecting CI environments.
4 participants