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

with JS files from etna-js #57

Merged
merged 13 commits into from
Jun 17, 2020
Merged

with JS files from etna-js #57

merged 13 commits into from
Jun 17, 2020

Conversation

coleshaw
Copy link
Collaborator

Shall we consolidate the shared code here, including JavaScript? As suggested in mountetna/etna-js#3?

NOTE: the GH Action for this PR only runs the NPM tests. We should set it up so that it runs both Ruby and JS tests.

@coleshaw coleshaw requested review from graft and corps June 17, 2020 00:56
@coleshaw
Copy link
Collaborator Author

@graft @corps , any final comments on this PR?

run: |
npm run test
- name: Run test suite
if: ${{ steps.npm_test_run.outcome == 'success' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be conditional? They could both run independently, in case JS tests are failing but we are working on ruby stuff...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm....I would expect at least for master, that both JS and Ruby tests pass, so that any work on a feature branch should have the other tests passing. I would be worried about, if you don't make the last step conditional, that only the last system status code is flagged as the returned status of this step, so a failure in the second Ruby tests would get missed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also run a completely separate action for Ruby tests (as we are for Ruby gem pushes, and we might for npm pushes some day).

However, it seems like if any of the individual steps fail the whole action is marked as failing, and since you are running them as separate steps, the two steps could run independently, and only if both pass would the action be green?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like separate the separate action idea...

README.md Outdated
@@ -1,150 +1,8 @@
# Mount Etna common library
These JavaScript components are shared across Mount Etna projects. You can install them via NPM into your project with: `npm install --save git+ssh://git@github.com:mountetna/etna-js.git`
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be out of date here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it appears to be overwriting the existing README :) Most of the existing README should move to mountetna.github.io anyway though...

@@ -0,0 +1,29 @@
## Upload Cycle Description.
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be deleted, it is very out of date.

@graft
Copy link
Contributor

graft commented Jun 17, 2020

The JS tests are very nicely written and cover a lot of important functionality, great work.

Let's 🚢 so we can use this elsewhere and build further.

@coleshaw coleshaw merged commit bb5c0cd into master Jun 17, 2020
@coleshaw coleshaw deleted the cs-merge-js-code branch June 17, 2020 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants