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

docs: make GHA default, docker optional #482

Closed
casperdcl opened this issue Apr 20, 2021 · 10 comments · Fixed by #483 or #488
Closed

docs: make GHA default, docker optional #482

casperdcl opened this issue Apr 20, 2021 · 10 comments · Fixed by #483 or #488
Assignees
Labels
documentation Markdown files

Comments

@casperdcl
Copy link
Contributor

casperdcl commented Apr 20, 2021

See e.g. https://partner.github.com/integration-resources/2020/11/24/pattern-setting-up-a-cli-on-github-s-hosted-runners.html

Would make it super easy to set up:

jobs:
  run:
    runs-on: [ubuntu-latest]
    steps:
      - uses: actions/checkout@v2
      - uses: iterative/cml@v1
      - name: 'Train my model'
        ...
        cml-send-comment report.md

EDIT:

- uses: actions/setup-node@v1
- uses: iterative/setup-cml@v1

works well but needs better documentation. Any objections to making it more prominent than container: docker://? Would fix e.g. #360

@0x2b3bfa0
Copy link
Member

👋 Hi, @casperdcl! Is uses: iterative/setup-cml@v1 what you are looking for?

@0x2b3bfa0 0x2b3bfa0 self-assigned this Apr 21, 2021
@0x2b3bfa0 0x2b3bfa0 added the awaiting-response Waiting for user feedback label Apr 21, 2021
@casperdcl
Copy link
Contributor Author

Aha yes; seems the docs really push for using docker instead.

I would discourage that thoroughly. There's no way people with complex build envs (most users?) would prefer a custom docker image over a native action.

@0x2b3bfa0 0x2b3bfa0 removed the awaiting-response Waiting for user feedback label Apr 21, 2021
@0x2b3bfa0
Copy link
Member

Agreed! If I recall correctly, @DavidGOrtega recommended Docker since the beginning because of isolation, but maybe we can explicitly offer both alternatives.

@casperdcl
Copy link
Contributor Author

casperdcl commented Apr 21, 2021

Docker's a great option, but is far from a canonical use case of GHA. Even if docker is used, it probably refers to action.yml using docker rather than a workflow doing container: docker://...

@casperdcl casperdcl changed the title native GHA docs: make GHA default, docker optional Apr 21, 2021
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Apr 21, 2021

Definitely, this may be worth a few documentation updates. What do you think, @DavidGOrtega?

Docker's a great option, but is far from a canonical use case of GHA.

Agreed! At least, when used for the same purposes as the usual setup-$TOOL actions you mentioned. I believe that the only advantage that led us to recommend Docker by default is environment isolation for both network and filesystem on cloud runners.

Even if docker is used, it probably refers to action.yml using docker rather than a workflow doing container: docker://...

Our GitHub Action is JavaScript-based and doesn't use Docker at all; it wouldn't add any value to the current use cases.

@casperdcl
Copy link
Contributor Author

Our GitHub Action is JavaScript-based and doesn't use Docker at all; it wouldn't add any value to the current use cases.

Yes I know, I'm not suggesting changing action.yml. I just mean that container: docker:// is fairly unexpected even for GHA users who know about Docker.

@casperdcl
Copy link
Contributor Author

I think the current README is a bit outdated and repetitive, hiding important info.

Missing and/or hidden things:

I can open a PR for more concrete discussion

/CC @shcheklein

@0x2b3bfa0
Copy link
Member

I just mean that container: docker:// is fairly unexpected even for GHA users who know about Docker.

👍 It's practical for some use cases, but not specially common among users, and probably not a sensible default for many complex workflows.

I think the current README is a bit outdated and repetitive, hiding important info.

Version 0.3.0 introduced some drastic changes, so I wouldn't be surprised if certain sections leave room for improvement. 😅

I can open a PR for more concrete discussion

That would be awesome!

@0x2b3bfa0 0x2b3bfa0 added the documentation Markdown files label Apr 21, 2021
@DavidGOrtega
Copy link
Contributor

@casperdcl @0x2b3bfa0
actions/setup-node@v1 might not be needed at all since its de facto in GHA.

I encourage using docker of course but not our image in GHA but the user's. Thats why we changed CML a bit.
Our cml.dev examples pushes the user to use the action instead our container but seems that we forgot this here?

I wont think that this fix #360 unfortunately since we have to support Gitlab and that job (better tagging) has to be done anyway.

@casperdcl
Copy link
Contributor Author

casperdcl commented Apr 22, 2021

Well actions/setup-node@v1 is needed if you want to pin the version or often needed in self-hosted but I'm sure you knew that. Will certainly point it out in the docs...

I think the usual point of container: docker:// is for end-users to define their own containers. CML's containers are to me more of a proof-of-concept than something we should be actively recommending people use.

This was referenced Apr 27, 2021
@casperdcl casperdcl linked a pull request Apr 29, 2021 that will close this issue
@0x2b3bfa0 0x2b3bfa0 assigned casperdcl and unassigned 0x2b3bfa0 May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Markdown files
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants