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

fix: Use correct mount path for create-syncstate-directory init container #93

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

kayano
Copy link
Contributor

@kayano kayano commented Dec 18, 2024

Instead hardcoded /data/das-storage mount path use the one provided in config for das helm chart

@kayano kayano requested a review from a team as a code owner December 18, 2024 20:57
a-thomas-22
a-thomas-22 previously approved these changes Dec 18, 2024
Copy link
Collaborator

@a-thomas-22 a-thomas-22 left a comment

Choose a reason for hiding this comment

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

looks good to me. Thanks!

@a-thomas-22 a-thomas-22 dismissed their stale review December 18, 2024 22:17

CI is not happy. I'll take a look and see if it is the PR or just our CI in general

@kayano
Copy link
Contributor Author

kayano commented Dec 18, 2024

@a-thomas-22 from what I can see CI (https://github.com/OffchainLabs/community-helm-charts/actions/runs/12401193859) is failing on installing das chart with values https://github.com/OffchainLabs/community-helm-charts/blob/main/charts/das/ci/sepolia-dac-minimal-values.yaml#L5-L11 so without init container create-syncstate-directory I'm changing volume mount path for as this https://github.com/OffchainLabs/community-helm-charts/blob/main/charts/das/templates/statefulset.yaml#L43 is not true so I don't understand how my change could break the CI as it is not used in that case.
Can we rerun it?

@a-thomas-22
Copy link
Collaborator

@a-thomas-22 from what I can see CI (https://github.com/OffchainLabs/community-helm-charts/actions/runs/12401193859) is failing on installing das chart with values https://github.com/OffchainLabs/community-helm-charts/blob/main/charts/das/ci/sepolia-dac-minimal-values.yaml#L5-L11 so without init container create-syncstate-directory I'm changing volume mount path for as this https://github.com/OffchainLabs/community-helm-charts/blob/main/charts/das/templates/statefulset.yaml#L43 is not true so I don't understand how my change could break the CI as it is not used in that case. Can we rerun it?

Ah, of course, it is because your PR can't access the required secrets. I ran the tests locally and all good!

Copy link
Collaborator

@a-thomas-22 a-thomas-22 left a comment

Choose a reason for hiding this comment

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

lgtm

@a-thomas-22 a-thomas-22 merged commit 0db0d59 into OffchainLabs:main Dec 19, 2024
2 of 4 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.

2 participants