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

Rework manifest install logic #523

Merged
merged 2 commits into from
Aug 14, 2024
Merged

Rework manifest install logic #523

merged 2 commits into from
Aug 14, 2024

Conversation

dbw7
Copy link
Contributor

@dbw7 dbw7 commented Aug 9, 2024

Closes #491

@dbw7 dbw7 requested a review from atanasdinov August 9, 2024 17:18
Copy link
Contributor

@atanasdinov atanasdinov left a comment

Choose a reason for hiding this comment

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

The solution looks good. I brought up some ideas for improvement - let me know what you think.

pkg/combustion/templates/k3s-multi-node-installer.sh.tpl Outdated Show resolved Hide resolved
@@ -38,6 +38,28 @@ CONFIGFILE={{ .configFilePath }}/{{ .initialiserConfigFile }}
mkdir -p /opt/eib-k8s/manifests
cp {{ .manifestsPath }}/* /opt/eib-k8s/manifests/

cat <<- 'EOF' > /opt/eib-k8s/create_manifests.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we can extract this as a template. It's pretty much the same across all files with the only difference being the path to the Kubernetes config. Would it be too much work?

Copy link
Contributor

@atanasdinov atanasdinov left a comment

Choose a reason for hiding this comment

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

LGTM! Did you have the time to think about extracting the script as a template?

@dbw7 dbw7 merged commit fa484c8 into suse-edge:main Aug 14, 2024
3 checks passed
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.

Large Helm manifests unable to install
2 participants