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

New CI #72

Merged
merged 16 commits into from
Jul 8, 2020
Merged

New CI #72

merged 16 commits into from
Jul 8, 2020

Conversation

stefanotorresi
Copy link
Collaborator

@stefanotorresi stefanotorresi commented Jul 6, 2020

Sister PR of ClusterLabs/ha_cluster_exporter#163, most of the code was copy-pasted from there.

@stefanotorresi stefanotorresi marked this pull request as ready for review July 7, 2020 17:30
@stefanotorresi
Copy link
Collaborator Author

stefanotorresi commented Jul 7, 2020

this is now ready for review , but please defer merging until the secrets are properly set in GH.

Copy link
Collaborator

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

@stefanotorresi I have not checked deeply, but it looks fine.
Some personal thoughts:

  • The travis badge in the README should be replaced by a github one (if it exists)

  • Now it looks like the changes file is updated from the github release, but we still have the changes file in the source code. How is this working? Maybe we could describe this in the README. Personally, I'm not a big fan of this process (update changes fro GH), but let's see how it goes

  • How is the python matrix working? I see that you define the python versions in the tox file and in the github workflow file.

  • I see that almost all the osc operations are done in the make file. The container for the CI was created exactly for that, to not duplicate the code in every project we have. If the commands go here it doesn't have sense to use the container anymore. I would use one or the other approach, but the combination of both looks strange.

  • Now the CI looks much more complicated than before, essentially because more files are involved. I guess that these files won't be changed at all, so it should be fine

.github/workflows/dashboards-ci.yml Outdated Show resolved Hide resolved
.github/workflows/dashboards-ci.yml Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
@stefanotorresi
Copy link
Collaborator Author

stefanotorresi commented Jul 8, 2020

Thanks for the feedback, Xabi!

  • The travis badge in the README should be replaced by a github one (if it exists)

Done! For the time being, I didn't replace the codeclimate one, though. I will explore this in the future for all the exporters.

  • Now it looks like the changes file is updated from the github release, but we still have the changes file in the source code. How is this working? Maybe we could describe this in the README. Personally, I'm not a big fan of this process (update changes fro GH), but let's see how it goes

This was actually a mistake on my part: I have removed the changes file now, because it is managed automatically. This only applies to the exporter, mind you.

I have left the README mostly un-touched here, but I have another PR coming up that overhauls it entirely, because some instructions are outdated.

  • How is the python matrix working? I see that you define the python versions in the tox file and in the github workflow file.

The two are not strictly related. We run the test job in both python 3.6 and 3.8. We use the tox py env to automatically get the currently installed python version. The other tox envs are only useful to run tests locally in systems with multiple python versions installed.

  • I see that almost all the osc operations are done in the make file. The container for the CI was created exactly for that, to not duplicate the code in every project we have. If the commands go here it doesn't have sense to use the container anymore. I would use one or the other approach, but the combination of both looks strange.

The container is still needed to provide an environment with osc installed and configured. I haven't yet had the chance to move what is being done in the make targets into the scripts within the container, but this would certainly be a very welcomed improvement! For the time being, copy-pasting was the simplest approach.
The way forward would probably be to create a custom reusable GitHub action (which at the end of the day is just a container and a bash script with a specific interface).

  • Now the CI looks much more complicated than before, essentially because more files are involved. I guess that these files won't be changed at all, so it should be fine

Heh, that's the trade-off for using the same repo for two packages, but I think it's worth it. Making related changes to both the exporter and the dashboards is a much smoother process now, e.g. see ClusterLabs/ha_cluster_exporter#167.

Copy link
Collaborator

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying everything @stefanotorresi
I think we are ready to go!

@stefanotorresi stefanotorresi merged commit 756e4e3 into SUSE:master Jul 8, 2020
@stefanotorresi stefanotorresi deleted the feature/new-ci branch July 8, 2020 11:25
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.

2 participants