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.

Sign up for GitHub

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

Jump to bottom

feat(multi-cluster): add multi-cluster support for all node commands #1416

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

feat(multi-cluster): add multi-cluster support for all node commands #1416

wants to merge 26 commits into from
+479 −350

Conversation

instamenta
Copy link
Contributor

@instamenta instamenta commented Feb 17, 2025 •
edited
Loading

Description

  • Modify all node commands

Related Issues

@instamenta instamenta self-assigned this Feb 17, 2025
@instamenta instamenta requested review from leninmehedy and a team as code owners February 17, 2025 14:32
@instamenta instamenta linked an issue Feb 17, 2025 that may be closed by this pull request
fix(multi-cluster): prevent nodes from being deployed to all clusters instead of the one specified #1408
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 17, 2025 •
edited
Loading

Unit Test Results - Linux

  1 files  ±0   62 suites  ±0   3s ⏱️ -1s
241 tests ±0  241 ✅ ±0  0 💤 ±0  0 ❌ ±0 
250 runs  ±0  250 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4b6a5ed. ± Comparison against base commit d2f7b21.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 17, 2025 •
edited
Loading

Unit Test Results - Windows

  1 files  ±0   62 suites  ±0   2s ⏱️ -2s
241 tests ±0  241 ✅ ±0  0 💤 ±0  0 ❌ ±0 
250 runs  ±0  250 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4b6a5ed. ± Comparison against base commit d2f7b21.

♻️ This comment has been updated with latest results.

@instamenta
Merge remote-tracking branch 'origin/main' into 01408-fixmulti-cluste…
02f1566 
…r-prevent-nodes-from-being-deployed-to-all-clusters-instead-of-the-one-specified
@instamenta instamenta marked this pull request as draft February 18, 2025 08:46
@instamenta
Merge remote-tracking branch 'origin/main' into 01408-fixmulti-cluste…
c98c650 
…r-prevent-nodes-from-being-deployed-to-all-clusters-instead-of-the-one-specified
@instamenta
added multi cluster support for all node commands
50411a9 
Signed-off-by: instamenta <[email protected]>
@instamenta instamenta marked this pull request as ready for review February 19, 2025 11:57
@instamenta instamenta changed the title feat(multi-cluster): modify the node commands 'updateChartWithConfigMap' task to work with multi-cluster setup feat(multi-cluster): add multi-cluster support for all node commands Feb 19, 2025
@instamenta
remove comments, fix failing test and start working on new method for…
8e59827 
… adding node to the remote config

Signed-off-by: instamenta <[email protected]>
@instamenta
lint-fix
f48a15d 
Signed-off-by: Zhan Milenkov <[email protected]>
@instamenta
Merge remote-tracking branch 'origin/main' into 01408-fixmulti-cluste…
0882373 
…r-prevent-nodes-from-being-deployed-to-all-clusters-instead-of-the-one-specified
@instamenta
add new method to executeAdd command task list
f65ae6f 
Signed-off-by: Zhan Milenkov <[email protected]>
@instamenta
temporary fix
a6f430f 
Signed-off-by: Zhan Milenkov <[email protected]>
@instamenta
try to fix tests
8764fcd 
Signed-off-by: Zhan Milenkov <[email protected]>
@jeromy-cannon jeromy-cannon added the PR: Checks Failed A pull request where the checks have failed. label Feb 20, 2025
@instamenta
remove promise all
56727ac 
Signed-off-by: Zhan Milenkov <[email protected]>
@instamenta
Merge remote-tracking branch 'origin/main' into 01408-fixmulti-cluste…
92d16e6 
…r-prevent-nodes-from-being-deployed-to-all-clusters-instead-of-the-one-specified
@instamenta
Merge remote-tracking branch 'origin/main' into 01408-fixmulti-cluste…
295ae46 
…r-prevent-nodes-from-being-deployed-to-all-clusters-instead-of-the-one-specified

# Conflicts:
#	src/commands/network.ts
#	test/e2e/commands/node_upgrade.test.ts
#	test/e2e/commands/separate_node_upgrade.test.ts
@instamenta
try to fix 'node add' commands
b4743c0 
Signed-off-by: Zhan Milenkov <[email protected]>
@instamenta
fix circular dep
1c9c82d 
Signed-off-by: Zhan Milenkov <[email protected]>
@instamenta
fix getConsensusNodes method
e6dbecc 
Signed-off-by: Zhan Milenkov <[email protected]>
@instamenta
cleanup and attempt to fix chart upgrade on node add command
0162434 
Signed-off-by: Zhan Milenkov <[email protected]>
@instamenta
Merge remote-tracking branch 'origin/main' into 01408-fixmulti-cluste…
5ee5cb3 
…r-prevent-nodes-from-being-deployed-to-all-clusters-instead-of-the-one-specified
@instamenta
attempt to get test to pass
4efe11a 
Signed-off-by: Zhan Milenkov <[email protected]>
@JeffreyDallas
Copy link
Contributor

Some E2E test failed, and it is hard to pin down what changes broke the tests.
I am trying to split the PR to a few small ones so it will be easier to debug.

@JeffreyDallas
Copy link
Contributor

Some tests failed because there were some insistencies in the code how context is retrieved.
Some times it got from local config as cluster-1, for E2E tests, it is hard coded in test/data/local_config.yaml

userEmailAddress: [email protected]
soloVersion: '0.0.0'
deployments:
  deployment:
    clusters:
      - cluster-1
    namespace: solo-e2e
clusterRefs:
  cluster-1: context-1
  cluster-2: context-2

sometimes context is retrieved from flag value which is kind-solo-e2e

Then this led to unexisted key in the map this.localConfig.clusterRefs

@JeffreyDallas JeffreyDallas requested review from a team as code owners February 22, 2025 20:51
@JeffreyDallas
Merge commit 'd2f7b21902ef1ad24ad2681bfb1c4461f36fd34f' into 01408-fi…
4b6a5ed 
…xmulti-cluster-prevent-nodes-from-being-deployed-to-all-clusters-instead-of-the-one-specified
This was referenced Feb 22, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeffreyDallas JeffreyDallas JeffreyDallas left review comments

@leninmehedy leninmehedy Awaiting requested review from leninmehedy leninmehedy is a code owner

@rbarker-dev rbarker-dev Awaiting requested review from rbarker-dev rbarker-dev is a code owner automatically assigned from hashgraph/platform-ci

At least 1 approving review is required to merge this pull request.

Assignees

@instamenta instamenta

Labels
PR: Checks Failed A pull request where the checks have failed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix(multi-cluster): prevent nodes from being deployed to all clusters instead of the one specified
3 participants
@instamenta @JeffreyDallas @jeromy-cannon