Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Decaffeinate and maintenance #20

Merged
merged 7 commits into from
Jan 4, 2018
Merged

Decaffeinate and maintenance #20

merged 7 commits into from
Jan 4, 2018

Conversation

Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Jan 3, 2018

This PR accomplishes several things:

  • Decaffeinate the code 🔥☕️ 🎉
  • Utilize async/await where possible.
  • Bring in semantic-release and commitlint

I'm using this as a testbed for semantic-release in order to test it before larger use through AtomLinter. See AtomLinter/Meta#18 for a bit more information.

@keplersj / @pvdlg I'd appreciate a look over the semantic-release related bits (or the rest if you want 😛) to make sure I haven't missed something or that there isn't a better way to do things since I'm basically starting from scratch for CD here 😉.

Move from CoffeeScript to JavaScript so the code is actually readable,
and allowing things like `async`/`await`. Also moves the configuration
to a `configSchema` in the `package.json` file. Dependencies are
deferred till they are actually needed, with an opportunistic load
during an "idle" time.
Convert the specs to JavaScript and utilize `async`/`await` to clean
them up.
Update the Linter API to v2 so this provider isn't stuck in the past.
Add a configuration for `commitlint` that enforces Angular-like commit
messages in preparation for adding of `semantic-release`.
@Arcanemagus Arcanemagus self-assigned this Jan 3, 2018
@pvdlg
Copy link

pvdlg commented Jan 3, 2018

In travis.yml, after running ./build-package.sh I add to run export PATH=$PATH:~/atom/usr/bin otherwise apm was not in the PATH.

I see from your Travis build that you added the ATOM_ACCESS_TOKEN environment variable. You will also need to add the GH_TOKEN. See apm-config#github-authentication. You have to create a GitHub personal token with public_repo permission. The token has to be created with a user that has push access on your repo.

I see that you are using travis-deploy-once. That should work fine, but you can alternatively use Travis Build stages.

In your travis.yml:

- after_success:
-  - npm run travis-deploy-once "npm run semantic-release"

+ jobs:
+   include:
+     - stage: release
+       node_js: node
+       os: linux
+       after_success:
+         - npm run semantic-release

And in your package.json:

  "scripts": {
    "commitmsg": "commitlint -e $GIT_PARAMS",
    "semantic-release": "semantic-release",
-   "travis-deploy-once": "travis-deploy-once"
  },

This way you will have your 2 regular jobs running, and when they are both successful, the release job would run and do a release if necessary. It has the advantage of relying on Travis itself rather than the travis-deploy-once tool. You will have to run a 3rd job though.

If you decide to go with travis-deploy-once you have to add it to your devDependencies as it's currently missing.

@keplersj
Copy link

keplersj commented Jan 3, 2018

Looks great to me! Address what @pvdlg brought up and I think this will make a great standard for AtomLinter packages.

@Arcanemagus
Copy link
Member Author

In travis.yml, after running ./build-package.sh I add to run export PATH=$PATH:~/atom/usr/bin otherwise apm was not in the PATH.

Hmmm, I thought we had that exporting in that script, I'll fix it there (with a temporary workaround here) if not.

I see from your Travis build that you added the ATOM_ACCESS_TOKEN environment variable. You will also need to add the GH_TOKEN.

There are actually two secure lines in there, one is ATOM_ACCESS_TOKEN, the other is GITHUB_TOKEN with that permission.

I see that you are using travis-deploy-once. That should work fine, but you can alternatively use Travis Build stages.

I actually looked into that, but as apm would need to be available and setting it up in the deploy job would be duplicating most of what the build-project.sh script does, I just decided to do it in one go for now. I'm thinking of putting up a PR to atom/ci that splits the installation into a separate script since it's the most difficult part to get right. (Note, I'd already noted that in 99d478d 😉.)

Of course if I was going to go that far I'd probably just get this running correctly in CircleCI as I much prefer their infrastructure.

@Arcanemagus
Copy link
Member Author

If you decide to go with travis-deploy-once you have to add it to your devDependencies as it's currently missing.

Fixed btw, missed it in one of the rebases.

Utilize the new shared config for APM for `semantic-release` in order to
automate deployment from CI.

Notes: Uses `travis-deploy-once` instead of Build Stages since we would
need to reconfigure Atom in the deploy job if using Build Stages.
Update the TravisCI configuration to match the current `atom/ci`
recommendation.
@Arcanemagus
Copy link
Member Author

Arcanemagus commented Jan 4, 2018

@pvdlg Is there a way to see what changelog will be generated?


There are actually two secure lines in there, one is ATOM_ACCESS_TOKEN, the other is GITHUB_TOKEN with that permission.

I'd messed up the declaration, that's why only the second one was showing up.

@pvdlg
Copy link

pvdlg commented Jan 4, 2018

Hmmm, I thought we had that exporting in that script, I'll fix it there (with a temporary workaround here) if not.

I think it add apm to the PATH on OSX but not on Linux.

Note, I'd already noted that in 99d478d 😉.

Sorry I missed that :)

@pvdlg Is there a way to see what changelog will be generated?

You can run npm run semantic-release --dry-run that will display the version and the changelog to be release but will skip the release.

@pvdlg
Copy link

pvdlg commented Jan 4, 2018

semantic-release/apm-config#5 is fixed. Everything should be good to go I think. Let me know if any other issue happens.

Note: The following breaking change was implemented and documented in
3e6355b, before `semantic-release` was brought in.

BREAKING CHANGE:

The `executablePath` setting has been removed and is no longer
available. `linter-pug` will now use your project's local `pug-lint`
when one can be found using the standard `require.resolve`
implementation. If one can't be found local to your project then the
bundled one will be used as a fallback.
@Arcanemagus
Copy link
Member Author

The changelog needs some work (every newline is a new breaking change???), but it will work "good enough" for this small test, merging!

@Arcanemagus Arcanemagus merged commit 8d4f68e into master Jan 4, 2018
@Arcanemagus Arcanemagus deleted the updates branch January 4, 2018 17:02
@Arcanemagus
Copy link
Member Author

Hmmm, so the last build for this PR passed the checks (here), but the actual release attempt failed to find apm here.

Starting a PR to run some tests, do you want an issue filed anywhere @pvdlg?

@pvdlg
Copy link

pvdlg commented Jan 4, 2018

The checks are not evaluated in a PR, we skip the release right away.

The release failed because apm is not in the PATH. You have to add export PATH=$PATH:~/atom/usr/bin after installing it.

If you push a commit that adds export PATH=$PATH:~/atom/usr/bin the release will be attempted again, including all the commits since the last release.

@Arcanemagus
Copy link
Member Author

The checks are not evaluated in a PR, we skip the release right away.

Hmmm, what do you think about running in --dry-run mode in PRs? That way the full tests could be performed, as well as providing a preview of the pending release.

@pvdlg
Copy link

pvdlg commented Jan 4, 2018

We skip everything in PR because the environment variables are not available in PR done from a downstream repo.
From Travis doc:

Encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code.

So we can't run anything in a PR as most verification check the validity of auth token and we can't access them.

The --dry-run is meant to test locally, with the auth token environment variables set in your machine.

Ideally we'd like to run the verification in PRs and inform the users about the release that will be performed by merging the PR, but that would require a GitHub App. See semantic-release/semantic-release#585.

@Arcanemagus
Copy link
Member Author

We skip everything in PR be cause the environment variables are not available in PR done from a downstream repo.

Excellent point, thanks for the issue reference!

@pvdlg
Copy link

pvdlg commented Jan 4, 2018

Regarding the change log,

The changelog needs some work (every newline is a new breaking change???), but it will work "good enough" for this small test, merging!

The changelog is generated by conventional-changelog. Issues/PR can be opened there for improvement.

If you have a suggestion for a better tool to generate the changelog we can certainly consider to use it in semantic-release.

In the current implementation, in my experience each no new line doesn't generate a breaking change. I usually format my breaking change commit like this:

feat: my feature

BREAKING CHANGE: General description of breaking change 1

Details explanation.

BREAKING CHANGE: General description of breaking change 2

Details explanation.

Maybe the problem happens if you have a new line directly after BREAKING CHANGE:

@pvdlg
Copy link

pvdlg commented Jan 4, 2018

Btw, regarding the apm PATH, we should probably open an issue with https://github.com/atom/ci

I can do it if you'd like.

@Arcanemagus
Copy link
Member Author

Btw, regarding the apm PATH, we should probably open an issue with https://github.com/atom/ci

I helped write most of those scripts, I'll be debugging that and thinking of a solution there 😉.

@Arcanemagus
Copy link
Member Author

@Arcanemagus
Copy link
Member Author

Btw @pvdlg it looks like the changelog is generated correctly for the actual release, the CLI gave me this:

### BREAKING CHANGES

    * The executablePath setting has been removed and is no longer
    * available. linter-pug will now use your project's local pug-lint
    * when one can be found using the standard require.resolve
    * implementation. If one can't be found local to your project then the
    * bundled one will be used as a fallback.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants