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

Set up github actions #482

Merged
merged 15 commits into from
May 29, 2020
Merged

Set up github actions #482

merged 15 commits into from
May 29, 2020

Conversation

bsamuel-ui
Copy link
Collaborator

@bsamuel-ui bsamuel-ui commented Feb 28, 2020

Addressing #468, this replaces CircleCI with Github Actions. There are most likely more elegant ways to restructure the tests, but I wanted to do as much of a straight port as possible. Definitely view the diff with ignore whitespace. 😬

Much thanks to @AndrewFarley for assistance with this.

I've got it working for 2.7 and 3.6, and I think this is a good point to try and merge it. 🤞

Outstanding issues:

dockerizePip fails on Windows. tryBindPath is looking for the alpine docker image to run its test, and it doesn't find that architecture. At least that message is visible in debug logs now.

Many 2.7 and 3.6 specific tests. I think tests should all be python agnostic as much as humanly possible. That mostly means dropping a bunch of tests and inferring the correct setting for --runtime

Tests do not need to be editing files. There's a teardown method that was checking out a test file, that's just a bad idea. Serverless has a powerful configuration system, we should look that value up through an option.

Copy link
Collaborator

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

Initial reaction: Looking cool - glad that it's working! I haven't been able to review this just yet - would it make sense to rebase/squash/reorganize your commits to be a little more reviewable? 32 commits and a diffstat of +1,686 −1,294 is a bit hard to consume in its current form.

Copy link
Collaborator

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

A bunch of inline comments. I haven't exhaustively covered the test.js changes.

.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Show resolved Hide resolved
- name: Test
run: npm run test
env:
USE_PYTHON: python
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this environment variable control?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So... that's a bit complicated. I'd like to chat about that on slack.

README.md Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@miketheman
Copy link
Collaborator

@bsamuel-ui It's been a while since this was reviewed- have you had any time to make progress on this front? It's okay if you haven't - let me know, and I'd be open to continue with this.

bsamuel-ui and others added 9 commits May 4, 2020 13:57
- Restrict lint to only run once
- Add skip clauses for python versions
Check that the mapping isn't empty and the python command didn't return null stdout.
- Use tape-promise and async tests.
Keep the eslint version fresh, so we don't fall too far behind and have
trouble updating later.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
During the evaluation in the package phase, we determine whether a
`requirements.txt` file exists, or whether we need to generate one.

Since the `pyproject.toml` file is used by poetry, but only if a stanza
is contained inside the file, use the function `isPoetryProject()` along
with the configuration value, thereby reducing the need for a project to
have to declare a configuration override.

Refs #324
Refs #344
Fixes #400

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
- Also restrict prettier to 1.x until we've closed out some PRs.
Remove circle config. Skip DockerizePip tests on windows.
- Drop .eslintrc.js in favor of package.json
- Remove aspirational comments.
- Correct README.
- Don't increase usage of lodash.
Copy link
Collaborator

@AndrewFarley AndrewFarley left a comment

Choose a reason for hiding this comment

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

LGTM. Did not try the tests locally, but reviewed the tests in Github Actions, and all seems pretty kosher. It nicely runs the lint and the tests, omitting certain tests for now that we know don't work, or that just don't work on windows. These failing tests are nice entry-points for new programmers to jump in and submit merge requests.

ACK

@bsamuel-ui bsamuel-ui merged commit abd6153 into master May 29, 2020
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.

3 participants