Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Adding ci with atom-beta on CircleCI #62

Merged
merged 4 commits into from
Aug 15, 2017

Conversation

kn1kn1
Copy link
Contributor

@kn1kn1 kn1kn1 commented Jan 13, 2017

This PR adds ci with atom-beta on CircleCI.

Adding environment variable ATOM_CHANNEL: "beta" to circle.yml enables ci with atom-beta.

dependencies:
  override:
    - curl -s -O https://raw.githubusercontent.com/atom/ci/master/build-package.sh
    - chmod u+x build-package.sh

test:
  override:
    - ./build-package.sh

machine:
  environment:
    ATOM_LINT_WITH_BUNDLED_NODE: "true"
    APM_TEST_PACKAGES: ""
    ATOM_CHANNEL: "beta"

Please note: this change will not affect current circle.yml for atom stable version.

@steelbrain
Copy link

A friendly bump, it'll be nice to have this land sooner. I have to maintain a build-package.sh separately on a gist because of this

@Arcanemagus
Copy link
Contributor

@kn1kn1 This has merge conflicts that need to be fixed btw.

@kn1kn1
Copy link
Contributor Author

kn1kn1 commented Aug 8, 2017

resolved.

CircleCI result:
https://circleci.com/gh/kn1kn1/language-context-free/327

Copy link
Contributor

@Arcanemagus Arcanemagus left a comment

Choose a reason for hiding this comment

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

Can you include specification of the ATOM_CHANNEL variable in the circle.yml file to expose this capability there?

@kn1kn1
Copy link
Contributor Author

kn1kn1 commented Aug 9, 2017

Can this comment be suitable for the spec or any alternatives?

dependencies:
  override:
    - curl -s -O https://raw.githubusercontent.com/atom/ci/master/build-package.sh
    - chmod u+x build-package.sh

test:
  override:
    - ./build-package.sh

machine:
  environment:
    ATOM_LINT_WITH_BUNDLED_NODE: "true"
    APM_TEST_PACKAGES: ""
    # ATOM_CHANNEL: "beta"  # "stable" or "beta". Default is "stable".

@Arcanemagus
Copy link
Contributor

I would just do:

# ... stuff
machine:
  environment:
    ATOM_LINT_WITH_BUNDLED_NODE: "true"
    APM_TEST_PACKAGES: ""
    ATOM_CHANNEL: "stable"

The other configs work the same way (explicitly setting stable).

kn1kn1 added a commit to kn1kn1/language-context-free that referenced this pull request Aug 9, 2017
@kn1kn1
Copy link
Contributor Author

kn1kn1 commented Aug 9, 2017

@Arcanemagus Thanks. I did so.

CircleCI result: https://circleci.com/gh/kn1kn1/language-context-free/329

Copy link
Contributor

@Arcanemagus Arcanemagus left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -49,8 +49,13 @@ elif [ "${CIRCLECI}" = "true" ]; then
sudo dpkg --install atom-amd64.deb || true
sudo apt-get update
sudo apt-get --fix-broken --assume-yes --quiet install
export ATOM_SCRIPT_PATH="atom"
export APM_SCRIPT_PATH="apm"
if [ "${ATOM_CHANNEL}" = "stable" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I would invert the logic here, so that if ATOM_CHANNEL is unset, stable is implicitly used.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's unset it defaults to stable:

ATOM_CHANNEL="${ATOM_CHANNEL:=stable}"

@kn1kn1
Copy link
Contributor Author

kn1kn1 commented Aug 15, 2017

Is there anything else for this pr to be merged?

@Arcanemagus
Copy link
Contributor

This is especially relevant now that CircleCI 2.0 is out of beta and you can do nice things like setup workflows where you build stable and beta in parallel (like Travis CI does).

@kn1kn1
Copy link
Contributor Author

kn1kn1 commented Aug 15, 2017

You'd better tell me that earlier as CircleCi 2.0 beta is out last month.

@kn1kn1 kn1kn1 closed this Aug 15, 2017
@Arcanemagus
Copy link
Contributor

@kn1kn1 Any reason you closed this? The fixes in here are still necessary to support beta builds!

@kn1kn1
Copy link
Contributor Author

kn1kn1 commented Aug 15, 2017

@Arcanemagus If you really think so, you can open another pr.

@joefitzgerald
Copy link
Contributor

@kn1kn1 it sounds like you've been put off by the conversation in this PR. If I have contributed to that in any way, my apologies. We do value the contribution! Merging.

@joefitzgerald joefitzgerald reopened this Aug 15, 2017
@joefitzgerald joefitzgerald merged commit ddf83c3 into atom:master Aug 15, 2017
@Arcanemagus
Copy link
Contributor

Thanks @joefitzgerald!

@kn1kn1 kn1kn1 deleted the feature/atom-beta-circleci branch August 17, 2017 12:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants