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

#561 replace travis with GitHub action #562

Merged
merged 8 commits into from
Oct 13, 2020
Merged

#561 replace travis with GitHub action #562

merged 8 commits into from
Oct 13, 2020

Conversation

Gontrum
Copy link
Contributor

@Gontrum Gontrum commented Oct 12, 2020

Hi everyone,

I'm not 100% sure if it fits like this. Actually I wanted to resolve #561 . Maybe you have some tips for me:

Does it fit otherwise?
Best regards

David Gontrum added 2 commits October 12, 2020 14:36
- renamed travis script in `package.json` to `github-build`
Copy link
Collaborator

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Overall looks good to me but not sure why the CI job isn't starting.

@Gontrum do you have the job running on your own fork ? If so can you post a link to the logs.

.github/workflows/html5sortable.yml Outdated Show resolved Hide resolved
@Gontrum
Copy link
Contributor Author

Gontrum commented Oct 12, 2020

I changed the version declaration to a string. Otherwise the actions always takes node version 10 ;-)
Sorry for that!

David Gontrum added 2 commits October 12, 2020 19:26
- renamed travis script in `package.json` to `github-build`
- removed greenkeeper-lockfile from actions, because https://github.com/greenkeeperio/greenkeeper-lockfile says, that greenkeeper has build-in support, now
- updated to node version 14, because we used it in travis, too
- we need a string for node-version, otherwise it will take version 10 automatically
- added some globals to make standard green
- added coveralls action to github action configuration
- added es-disable an export in `store.ts` to  make standard green
- added HTMLElement to global to make the standard green
- changed syntax for creating lcov
- removed `.coveralls.yml` because we dont need it anymore
- removed accidentally added reporter from `package.json`
- we dont need coveralls anymore because of the action, so I removed it completely as a dependency
changed badge to github actions instead of travis
@lukasoppermann
Copy link
Owner

Hey @atodorov this should satisfy your change request so we can merge it, correct?

Copy link
Collaborator

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

LGTM now

@atodorov atodorov changed the title [WIP] #561 replace travis with GitHub action #561 replace travis with GitHub action Oct 13, 2020
@atodorov
Copy link
Collaborator

@Gontrum can you rebase on top of the latest master and resolve the conflicts please.

David Gontrum added 2 commits October 13, 2020 09:23
- renamed travis script in `package.json` to `github-build`
- removed greenkeeper-lockfile from actions, because https://github.com/greenkeeperio/greenkeeper-lockfile says, that greenkeeper has build-in support, now
- updated to node version 14, because we used it in travis, too
- we need a string for node-version, otherwise it will take version 10 automatically
- added some globals to make standard green
- added coveralls action to github action configuration
- added es-disable an export in `store.ts` to  make standard green
- added HTMLElement to global to make the standard green
- changed syntax for creating lcov
- removed `.coveralls.yml` because we dont need it anymore
- removed accidentally added reporter from `package.json`
- we dont need coveralls anymore because of the action, so I removed it completely as a dependency
- changed badge to github actions instead of travis
@Gontrum
Copy link
Contributor Author

Gontrum commented Oct 13, 2020

@Gontrum can you rebase on top of the latest master and resolve the conflicts please.

Sure :-)
Should be clean, now. Thank you guys for your help.

@atodorov
Copy link
Collaborator

@lukasoppermann I don't see any obvious conflicts here but I can't "Rebase & merge". The only available options for me are "Merge pull request (with a merge commit)" or "Squash and merge".

@lukasoppermann
Copy link
Owner

@atodorov same here. I always "squash & merge" in any case, as it keeps the history a bit more clean & understandable. Does anything speak against squash & merge?

@atodorov atodorov merged commit 671acc3 into lukasoppermann:master Oct 13, 2020
@atodorov
Copy link
Collaborator

@atodorov same here. I always "squash & merge" in any case, as it keeps the history a bit more clean & understandable. Does anything speak against squash & merge?

FTR: I think the issue here was a merge commit in the PR itself.

I personally prefer "Rebase & merge" to keep the history clear and also keep the individual commits separate so we can trace back history more granularly. In this particular case I agree squash was the right thing to do since there were many commits fixing items from the code review process or adding changes missing from the original commit.

@Gontrum
Copy link
Contributor Author

Gontrum commented Oct 13, 2020

@atodorov thanks for the hint with the merge commit, I will keep that in mind.

@lukasoppermann
Copy link
Owner

@atodorov so if a PR has multiple commits that make sense by themselves I agree with the "Rebase & merge". Often though the PR history is not cleaned up. But we could ask for a clean history as well. 👍

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.

Replace Travis with github action
3 participants