gazebo-tooling / action-gz-ci

MIT License
5 stars 4 forks source link

Remove Ubuntu version from this action's name #16

Closed chapulina closed 4 years ago

chapulina commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

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 commented 4 years ago

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!