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

Merge elemental installer #20

Merged
merged 7 commits into from
Jul 7, 2022
Merged

Merge elemental installer #20

merged 7 commits into from
Jul 7, 2022

Conversation

davidcassany
Copy link
Contributor

@davidcassany davidcassany commented Jul 7, 2022

This PR includes the former ros-installer code in this repository. ros-installer is then called elemental-installer.

Related to #7

@davidcassany davidcassany marked this pull request as draft July 7, 2022 08:59
@davidcassany davidcassany force-pushed the merge_elemental_installer branch from 4bc6696 to 520efc7 Compare July 7, 2022 09:08
Signed-off-by: David Cassany <david@localhost.localdomain>
Signed-off-by: David Cassany <david@localhost.localdomain>
@davidcassany davidcassany force-pushed the merge_elemental_installer branch from 520efc7 to 113819e Compare July 7, 2022 09:09
Signed-off-by: David Cassany <david@localhost.localdomain>
@davidcassany davidcassany force-pushed the merge_elemental_installer branch from 113819e to 4b69306 Compare July 7, 2022 09:22
Copy link
Contributor

@kkaempf kkaempf left a comment

Choose a reason for hiding this comment

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

lgtm
I will adapt isv:Rancher:Elemental/elemental-operator accordingly

@kkaempf
Copy link
Contributor

kkaempf commented Jul 7, 2022

Will you do the "rancheros" -> "elemental" renames in this PR or a separate one ?

@kkaempf
Copy link
Contributor

kkaempf commented Jul 7, 2022

https://build.opensuse.org/package/show/isv:Rancher:Elemental/elemental-operator is adapted and building fine ;-)

@davidcassany
Copy link
Contributor Author

davidcassany commented Jul 7, 2022

Will you do the "rancheros" -> "elemental" renames in this PR or a separate one ?

Gonna do it here, I was trying to see of if there was some way from elemental repository to consume this right away and attempt some manual checks, but probably is faster going through a regular build using the OSB image.

Changed my mind, see all rancheros references are were already part of the elemental-operator, there were no rancheros references in ros-installer code, hence I prefere to do the rename in a different PR just not to mix things and have a clean history. Looks like many things need to be addressed in operator and we are already few people around this code. Better step by step.

@davidcassany davidcassany marked this pull request as ready for review July 7, 2022 10:34
@davidcassany
Copy link
Contributor Author

I consider this ready to be merged. It is not actually tested yet, it is just a port form ros-installer, it builds and former ros-installer tests are all passing. Changes in former elemental-operator are really cosmetic, just a little project re-structure to move former main.go into a subfolder.

Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@Itxaka Itxaka left a comment

Choose a reason for hiding this comment

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

This requires changes to the goreleaser so it can build both binaries.
also to the Dockerfile so it picks the newer binary

davidcassany and others added 3 commits July 7, 2022 12:45
Co-authored-by: Itxaka <igarcia@suse.com>
Co-authored-by: Itxaka <igarcia@suse.com>
…ental-operator as they used to

Signed-off-by: David Cassany <dcassany@suse.com>
@@ -1,6 +1,7 @@
project_name: rancheros-operator
builds:
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also build the installer here as a different binary? Or is that for a post PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought doing it in a post PR

Copy link
Contributor

@Itxaka Itxaka left a comment

Choose a reason for hiding this comment

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

Not sure what I wrote here lol

@davidcassany
Copy link
Contributor Author

This requires changes to the goreleaser so it can build both binaries. also to the Dockerfile so it picks the newer binary

Yip, thanks! Now I just adapted it to keep releasing and building the elemental-operator as it was before. I have not yet included a releaser for both neither included a docker image for elemental-installer

@Itxaka
Copy link
Contributor

Itxaka commented Jul 7, 2022

lint,unit and e2e tests are being fixed in a different PR so it should not be a blocker to merge this.

Also we should consider following up on this with a config for the coverage to treat the installer and the operator as different packages, as Im pretty sure the installer has a 3% coverage or something like that lol

Copy link
Contributor

@kkaempf kkaempf left a comment

Choose a reason for hiding this comment

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

ship it !

@Itxaka
Copy link
Contributor

Itxaka commented Jul 7, 2022

umm, something wrong with the tests? Or its the test suite that big LOL

@davidcassany
Copy link
Contributor Author

umm, something wrong with the tests? Or its the test suite that big LOL

Oh, I think these tests can't be executed in parallell 😞

Signed-off-by: David Cassany <dcassany@suse.com>
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@211ad46). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #20   +/-   ##
=======================================
  Coverage        ?   43.27%           
=======================================
  Files           ?        8           
  Lines           ?      647           
  Branches        ?        0           
=======================================
  Hits            ?      280           
  Misses          ?      348           
  Partials        ?       19           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 211ad46...6d91be5. Read the comment docs.

@davidcassany
Copy link
Contributor Author

@Itxaka allright, now tets are passing and I am wondering about merging it, however probably #18 should be merged first and then rebase this one, which is smaller. What you think?

@Itxaka
Copy link
Contributor

Itxaka commented Jul 7, 2022

@davidcassany nah, Im still working on that one, will take some time still so lets merge this, I will rebase and fix on my side!

@davidcassany
Copy link
Contributor Author

davidcassany commented Jul 7, 2022

Ok then merging this one, then we can, at least, start building the elemental-installer binary

@davidcassany davidcassany merged commit 1d97f14 into main Jul 7, 2022
@davidcassany davidcassany deleted the merge_elemental_installer branch July 7, 2022 12:47
@kkaempf
Copy link
Contributor

kkaempf commented Jul 7, 2022

We should probably tag it with a version > 0.1.0 (last ros-installer release) !?

@davidcassany
Copy link
Contributor Author

We should probably tag it with a version > 0.1.0 (last ros-installer release) !?

yes makes sense, I think we could tag something like 0.1.y and then tag v0.2.x once we have all the renaming and smoke tests in place.

@kkaempf
Copy link
Contributor

kkaempf commented Jul 7, 2022

Please do, it makes my packaging task a bit easier 😉

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.

4 participants