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

Make metal3 helm chart compatible with the Sylva project #39

Merged
merged 8 commits into from
Oct 19, 2023

Conversation

ipetrov117
Copy link
Contributor

@ipetrov117 ipetrov117 commented Oct 15, 2023

This PR introduces changes that enable the metal3-deploy helm chart to be successfully deployed within the Sylva project.


Changes related to the MariaDB chart:

  1. MariaDB service no longer requires a hardcoded clusterIP for it to function.

Commits:


Changes related to the Ironic chart:

  1. Ironic's deployment templatization has been improved
  2. Ironic now communicates with MariaDB over the service FQDN
  3. Extend Ironic's service templatization

Commits:


Changes related to the Baremetal-operator chart:

  1. Templated security context for the BMO deployment
  2. Included clusterctl.cluster.x-k8s.io label to all CRDs in order to ensure that they can be pivoted when doing clusterctl move
  3. Removed unnecessary configurations from the BMO ConfigMap

Commits:

@ipetrov117 ipetrov117 changed the title Sylva compatibility Make metal3-deploy helm chart compatible with the Sylva project Oct 15, 2023
@hardys
Copy link
Contributor

hardys commented Oct 16, 2023

Overall this looks good, but in future IMHO it would be much better to break changes like this into several PRs since it contains several logically unrelated changes - this would be far easier/faster to review.

The main issue I see is the DATABASE_HOST change is backwards incompatible and won't work with the default chart configuration - I think that means we either add a conditional for emptydatabase_clusterIP or rebase on #37 which solves the underlying problem by removing the special dnsConfig from the Ironic Pod.

@ipetrov117
Copy link
Contributor Author

Overall this looks good, but in future IMHO it would be much better to break changes like this into several PRs since it contains several logically unrelated changes - this would be far easier/faster to review.

The main issue I see is the DATABASE_HOST change is backwards incompatible and won't work with the default chart configuration - I think that means we either add a conditional for emptydatabase_clusterIP or rebase on #37 which solves the underlying problem by removing the special dnsConfig from the Ironic Pod.

I mentioned it on #38, but just to cover here also. Merging #37 and #38 in 0.2.0 first would greatly benefit this PR. Furthermore by doing this we will keep 0.1.0 backwards compatible.

@ipetrov117 ipetrov117 changed the title Make metal3-deploy helm chart compatible with the Sylva project Make metal3 helm chart compatible with the Sylva project Oct 19, 2023
@ipetrov117
Copy link
Contributor Author

Overall this looks good, but in future IMHO it would be much better to break changes like this into several PRs since it contains several logically unrelated changes - this would be far easier/faster to review.
The main issue I see is the DATABASE_HOST change is backwards incompatible and won't work with the default chart configuration - I think that means we either add a conditional for emptydatabase_clusterIP or rebase on #37 which solves the underlying problem by removing the special dnsConfig from the Ironic Pod.

I mentioned it on #38, but just to cover here also. Merging #37 and #38 in 0.2.0 first would greatly benefit this PR. Furthermore by doing this we will keep 0.1.0 backwards compatible.

#37 and #38 have been merged and this PR has been rebased on top of them in the 0.2.0 version. Furthermore I added the missing packages needed to build the metal3 artefacts with the current changes.

Copy link
Contributor

@hardys hardys left a comment

Choose a reason for hiding this comment

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

Overall lgtm - couple of small comments/questions related to the configurable values.

@ipetrov117
Copy link
Contributor Author

Overall lgtm - couple of small comments/questions related to the configurable values.

Change suggestions have been addressed.

@hardys hardys self-requested a review October 19, 2023 15:14
@dprodanov4 dprodanov4 self-assigned this Oct 19, 2023
@hardys hardys merged commit 484b5c5 into suse-edge:main Oct 19, 2023
2 checks passed
@dprodanov4 dprodanov4 removed their assignment Oct 19, 2023
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.

3 participants