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

Convert docker-based action to typescript-based one #12

Conversation

KengoTODA
Copy link
Contributor

@KengoTODA KengoTODA commented Nov 18, 2020

To close #11, convert the docker-based action to typescript-based one, then we can use post to run the cleanup operation.

Note that the configurations for build tools are copied from the official template project. I hope that it reduces the barrier to contribute for other users.

this change will close saucelabs#11 by running `docker container stop` after
the workflow run.
Dockerfile and entrypoint.sh are now needless, simply remove
them from the Git repository.
Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

This is great! A couple of comments.

src/main.ts Outdated Show resolved Hide resolved
import optionMappingJson from './option-mapping.json'

const CONTAINER_VERSION = '4.6.2'
const LOG_FILE = '/srv/sauce-connect.log'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to upload this log file as asset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

possible, refs https://github.com/actions/toolkit/tree/main/packages/artifact

Is it OK to make this change in this PR? I assume that we can handle this new feature in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that we can handle this new feature in another PR.

Feel free to add it as part of this PR. If not, let's create an issue for it an tackle it afterwards.

src/post.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Does the way this action is used changed in any way? If so we should update the README accordingly and release with a new major version.

import {wait} from './wait'
import optionMappingJson from './option-mapping.json'

const CONTAINER_VERSION = '4.6.2'
Copy link
Contributor

Choose a reason for hiding this comment

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

Making this an GH Action option would be great. Let's do this in a separate PR though.

@KengoTODA
Copy link
Contributor Author

Does the way this action is used changed in any way? If so we should update the README accordingly and release with a new major version.

Input, output and behavior are not changed. Just fix #11. Performance could be improved.

Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you so much 👍

@christian-bromann christian-bromann merged commit 2545753 into saucelabs:master Nov 20, 2020
Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

I think I prematurely merged this PR. Can you revert the test command to also execute our e2e test?

"docker:tag": "docker tag saucelabs/sauce-connect-action saucelabs/sauce-connect-action:$npm_package_version",
"test": "run-p test:*",
"test:setup": "http-server ./test -p 8080 > /dev/null 2>&1 &",
"test:run": "node ./test/test.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get back this test?

Copy link
Contributor

Choose a reason for hiding this comment

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

#15

@KengoTODA KengoTODA deleted the convert-docker-based-to-typescript-based branch November 21, 2020 00:31
@tjenkinson
Copy link
Contributor

It looks like this also changes it to wait for the sc.ready file? Great job!

I was trying to figure out how to do this before with the docker version (so that we weren't waiting 30 seconds each time, and it would fail on an error) but was struggling to access the file. I think it was related to the directory being mounted on the host not the container.

@KengoTODA
Copy link
Contributor Author

Can you reproduce the issue?
In my environment, this implementation works without this problem. It'll be a great help to solve this issue for other users. :)

@tjenkinson
Copy link
Contributor

Sorry it works for us too with the current version (although it looks like the ready file gets written even on a concurrency error) :)

I was trying to achieve the same thing with the docker version but failed

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.

The container isn't terminated when the action run in self-hosted runner
3 participants