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

Remove Ubuntu version from this action's name #16

Closed
chapulina opened this issue Jul 21, 2020 · 7 comments
Closed

Remove Ubuntu version from this action's name #16

chapulina opened this issue Jul 21, 2020 · 7 comments
Assignees

Comments

@chapulina
Copy link
Contributor

I think it was a bad idea to include the Ubuntu version name in this action. The current code can be easily extended to other Ubuntu versions too. I propose we rename this to ubuntu-ci-action.

Thoughts, @j-rivero @mjcarroll ? Since we're making a lot of updates, this may be a good time time to just go ahead and make this change too. We should also move things from the .github/ci-bionic folder, maybe just to .github/ci?

@j-rivero
Copy link
Contributor

The current code can be easily extended to other Ubuntu versions too. I propose we rename this to ubuntu-ci-action.

+1. We could even remove Ubuntu from the name in same way that the tooling group has its name action-ros-ci. What about action-ignition-ci ?

@mjcarroll
Copy link
Contributor

I think I agree with this. The only consideration would be supporting multiple Ubuntu versions out of the same set of scripts. If we had eg .github/ci/pre_make.sh, would we expose variables in there to help with determining the platform? That would be a reason to potentially keep ci-bionic vs just ci.

@chapulina
Copy link
Contributor Author

If we had eg .github/ci/pre_make.sh, would we expose variables in there to help with determining the platform?

Yup, I think that would work. Another alternative would be exposing it as an action input. Maybe docker_image, defaulting to ubuntu:18.04?

@mjcarroll
Copy link
Contributor

I think there are two parts.

  1. We want to control which distribution it actually builds against. That makes sense as an action input.
  2. We want to control the execution of CI scripts based on distribution, this could be via an environment variable input

@j-rivero
Copy link
Contributor

I think there are two parts.

1. We want to control which distribution it actually builds against.  That makes sense as an action input.

2. We want to control the execution of CI scripts based on distribution, this could be via an environment variable input

+1

@chapulina
Copy link
Contributor Author

After trying a few different things, I didn't find a nice way to parametrize the docker image in a way that is contained within this action and keeps using: 'docker' . Things I learned:

  • There's no way to pass --build-args to docker build, which would allow us to get the docker_image input as an ARG and pass it to FROM.

  • I considered generating the Dockerfile from a template before building the image. That could maybe be done with another action like this one. But it's not possible to call an action from an action.

We want to control the execution of CI scripts based on distribution, this could be via an environment variable input

The first time I read this I thought I understood it, but now I'm not entirely sure what you mean by this @mjcarroll .

@chapulina
Copy link
Contributor Author

Renamed this repo to action-ignition-ci . Old actions still work, and we have support for focal on the focal branch.

Thanks for all the help!

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

No branches or pull requests

3 participants