feat: set --pvcs to true by default#4230
Conversation
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 36 |
🟢 Coverage 90.60% diff coverage · +0.03% coverage variation
Metric Results Coverage variation ✅ +0.03% coverage variation (-1.00%) Diff coverage ✅ 90.60% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (9ec1e20) 63178 51428 81.40% Head commit (580e73a) 63406 (+228) 51630 (+202) 81.43% (+0.03%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4230) 234 212 90.60% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Unit Test Results - Linux36 tests 36 ✅ 0s ⏱️ Results for commit 782270d. ♻️ This comment has been updated with latest results. |
Unit Test Results - Windows 1 files ±0 333 suites +1 9s ⏱️ -1s For more details on these failures, see this check. Results for commit ec32186. ± Comparison against base commit 733d052. ♻️ This comment has been updated with latest results. |
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
Ivo-Yankov
added
the
PR: Needs Manager Approval
E2E Test Report 10 files 94 suites 1h 18m 14s ⏱️ Results for commit 782270d. ♻️ This comment has been updated with latest results. |
jeromy-cannon
left a comment
jeromy-cannon
left a comment
There was a problem hiding this comment.
I altered the description of the GHI. if you want to merge this first, then do a relates to instead of closes on the GHI.
jeromy-cannon
added
PR: Unresolved Comments
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
Ivo-Yankov
changed the title
Ivo-Yankov
changed the title
| (config.resolvedPvcStorageClass | ||
| ? ` --set "defaults.volumeClaims.storageClassName=${config.resolvedPvcStorageClass}"` + | ||
| ` --set "minio-server.tenant.pools[0].storageClassName=${config.resolvedPvcStorageClass}"` | ||
| : ''); |
There was a problem hiding this comment.
I believe the current status is that we don't set this. If the storage class name is not set, then it will use the default. We want to maintain backwards compatibility as much as possible.
So, if the user does not supply a storageClassName then we want to match the existing behavior of enabling the PVCs and using the default storageClass assuming it is local-path-provider and that it exists (see more details from the ticket on exception scenarios)
| * Passing LOCAL_PATH_PROVISIONER as userSuppliedClass is treated the same as passing an empty | ||
| * string — both run the auto-detect cascade instead of validating by name. | ||
| */ | ||
| export async function resolveStorageClass(k8: K8, logger: SoloLogger, userSuppliedClass: string): Promise<string> { |
There was a problem hiding this comment.
we should use classes and static methods, we should not export functions.
| } | ||
| } | ||
|
|
||
| public async installManifest(filePath: string): Promise<void> { |
There was a problem hiding this comment.
this seems like a duplicate of applyManifest, perhaps you should just enhance that method if needed.
|
|
||
| return storageClasses; | ||
| } catch (error) { | ||
| throw new SoloError('Failed to list StorageClasses', error); |
There was a problem hiding this comment.
match K8 error handling logic
| `No StorageClass found in cluster — installing ${constants.LOCAL_PATH_PROVISIONER}-provisioner. ` + | ||
| 'Use --pvc-storage-class to specify an existing StorageClass.', | ||
| ); | ||
| await k8.manifests().installManifest(manifestPath); |
There was a problem hiding this comment.
I think you are missing to check for the default storage class and if none exists set this one as the default.
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
# Conflicts: # src/integration/kube/resources/manifest/manifests.ts
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
# Conflicts: # src/core/dependency-injection/inject-tokens.ts
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
# Conflicts: # src/commands/node/tasks.ts # test/unit/commands/node/local-build-path-pvc-validation.test.ts
Signed-off-by: Ivo Yankov <ivo@devlabs.bg>
jeromy-cannon
added
PR: Needs Team Approval
jan-milenkov
added
the
PR: Checks Failed
Ivo-Yankov
removed
the
PR: Needs Team Approval
|
This pull request is stale because it has been open 14 days with no activity. Remove stale label or comment on this pull request; otherwise, this pull request will be closed in 14 days. |
github-actions
Bot
added
the
Stale
# Conflicts: # src/commands/mirror-node.ts # src/commands/network.ts # version.ts
Ivo-Yankov
removed
the
Stale
jeromy-cannon
added
the
PR: Merge Conflicts
4 participants
Description
This PR introduces a breaking change. It changes the default value of
--pvcsto true for all commands that use it, and adds a new flag--pvc-storage-classtosolo consensus network deploy.The new flag is used to specify a storage class to be used by the PVCs. If it is not found in the cluster,
solois responsible for installing it. The default value israncher.io/local-path.This PR also improves the wording of an error. The original may be interpreted in a way that results in an
Unknown argumenterror.Converted to draft for further testing.
Related Issues
Pull request (PR) checklist
package.jsonchanges have been explained to and approved by a repository managerTesting
The following manual testing was done:
The following was not tested:
Commit message guidelines
We use 'Conventional Commits' to ensure that our commit messages are easy to read, follow a consistent format, and for automated release note generation. Please follow the guidelines below when writing your commit messages: