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

neofs-adm: add new command morph netmap-candidates #2529

Merged
merged 1 commit into from
Aug 30, 2023
Merged

neofs-adm: add new command morph netmap-candidates #2529

merged 1 commit into from
Aug 30, 2023

Conversation

AliceInHunterland
Copy link
Contributor

This command lists netmap candidates in the netmap contract. It allows checking if a node has been bootstrapped successfully.

Closes #1889.

@AliceInHunterland AliceInHunterland force-pushed the feature/1889-neofs-adm-morph-netmap-candidates branch from 537fccf to 37dfbf3 Compare August 28, 2023 15:16
@codecov
Copy link

codecov bot commented Aug 28, 2023 •
edited
Loading

Codecov Report

Merging #2529 (2f1c67a) into master (f5ad4a8) will decrease coverage by 0.03%.
The diff coverage is 6.25%.

❗ Current head 2f1c67a differs from pull request most recent head b621852. Consider uploading reports for the commit b621852 to get more accurate results

@@            Coverage Diff             @@
##           master    #2529      +/-   ##
==========================================
- Coverage   29.69%   29.66%   -0.03%     
==========================================
  Files         404      405       +1     
  Lines       30728    30756      +28     
==========================================
  Hits         9124     9124              
- Misses      20834    20862      +28     
  Partials      770      770
Files Changed Coverage Δ
...fs-adm/internal/modules/morph/netmap_candidates.go 0.00% <0.00%> (ø)
pkg/morph/client/netmap/netmap.go 30.76% <0.00%> (ø)
pkg/morph/client/netmap/snapshot.go 0.00% <0.00%> (ø)
cmd/neofs-adm/internal/modules/morph/root.go 50.00% <40.00%> (-0.30%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@AliceInHunterland AliceInHunterland force-pushed the feature/1889-neofs-adm-morph-netmap-candidates branch 2 times, most recently from 0135dfd to ada6219 Compare August 28, 2023 15:21
roman-khimov
roman-khimov reviewed Aug 28, 2023
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Looks OK otherwise.

cmd/neofs-adm/internal/modules/morph/netmap-candidates.go Outdated Show resolved Hide resolved
@AliceInHunterland AliceInHunterland force-pushed the feature/1889-neofs-adm-morph-netmap-candidates branch from ada6219 to 63b540b Compare August 29, 2023 13:51
roman-khimov
roman-khimov approved these changes Aug 29, 2023
View reviewed changes
cmd/internal/netmap.go Outdated Show resolved Hide resolved
@AliceInHunterland AliceInHunterland force-pushed the feature/1889-neofs-adm-morph-netmap-candidates branch from 63b540b to 84b48ae Compare August 29, 2023 14:25
carpawell
carpawell reviewed Aug 29, 2023
View reviewed changes
cmd/neofs-adm/internal/modules/morph/root.go Show resolved Hide resolved
cmd/internal/cmdprinter/netmap.go
@@ -1,4 +1,4 @@
package common
package cmdprinter
Copy link
Member

Choose a reason for hiding this comment

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

why do we need that renaming? why not a separate commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2529 (comment) and #2529 (comment) and it nearly only connected with netmap-candidates diff

Copy link
Member

Choose a reason for hiding this comment

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

so import cycles or what exactly made you move that file? not critical but just was surprised it is a required change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to use PrettyPrintNodeInfo in neofs-adm (netmap_candidates.go), so I could just copy this function, but with @roman-khimov we decided to move internal/common/netmap.go file from neofs-cli to avoid code repetition.

cmd/neofs-adm/internal/modules/morph/netmap_candidates.go Show resolved Hide resolved
cmd/neofs-adm/internal/modules/morph/netmap_candidates.go Show resolved Hide resolved
@AliceInHunterland AliceInHunterland force-pushed the feature/1889-neofs-adm-morph-netmap-candidates branch 3 times, most recently from 6fde5f6 to a8db51f Compare August 30, 2023 10:40
@AliceInHunterland
neofs-adm: add new command morph netmap-candidates
b621852 
This command lists netmap candidates in the netmap contract. It allows
checking if a node has been bootstrapped successfully.

Closes #1889.

Signed-off-by: Ekaterina Pavlova <ekt@morphbits.io>
@AliceInHunterland AliceInHunterland force-pushed the feature/1889-neofs-adm-morph-netmap-candidates branch from a8db51f to b621852 Compare August 30, 2023 13:06
@roman-khimov roman-khimov merged commit 5955244 into master Aug 30, 2023
6 of 8 checks passed
@roman-khimov roman-khimov deleted the feature/1889-neofs-adm-morph-netmap-candidates branch August 30, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

@carpawell carpawell carpawell approved these changes

@cthulhu-rider cthulhu-rider Awaiting requested review from cthulhu-rider cthulhu-rider is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Netmap candidates list in the neofs-adm
3 participants
@AliceInHunterland @roman-khimov @carpawell