Skip to content

feat: add identify option to swarm peers command#9745

Merged
Jorropo merged 16 commits intoipfs:masterfrom
arthurgavazza:feat/swarm-peers-identify
Mar 30, 2023
Merged

feat: add identify option to swarm peers command#9745
Jorropo merged 16 commits intoipfs:masterfrom
arthurgavazza:feat/swarm-peers-identify

Conversation

@arthurgavazza
Copy link
Copy Markdown
Contributor

It adds the feature requested on #9578. This PR adds a new opt-in flag called identify that adds Identify values to the command output.

Copy link
Copy Markdown
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

Thx for your patch, I see minor things.

arthurgavazza and others added 2 commits March 21, 2023 23:33
Co-authored-by: Jorropo <jorropo.pgm@gmail.com>
Co-authored-by: Jorropo <jorropo.pgm@gmail.com>
Copy link
Copy Markdown
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

LGTM thx, now tests please 🙂 ? (using the new go based system, not sharness)

Also CI is red (it is flaky, but not that much :D).

@arthurgavazza
Copy link
Copy Markdown
Contributor Author

@Jorropo Are there any tests for the swarmPeersCmd ? sorry for the obvious questions, that's my first issue and I couldn't find the tests for this command, so that I could add my own

@Jorropo
Copy link
Copy Markdown
Contributor

Jorropo commented Mar 22, 2023

@arthurgavazza here are the tests https://github.com/ipfs/kubo/blob/master/test/sharness/t0140-swarm.sh, this use the old sharness thing, you can add there but it's a pain, ideally you would use our newer go based test suite (https://github.com/ipfs/kubo/tree/master/test/cli) but that more work since you would be bootstrapping a new swarm test file.

@arthurgavazza
Copy link
Copy Markdown
Contributor Author

@Jorropo thx for the info! I'll try creating the tests using the new model, tomorrow I'll commit the updates

@arthurgavazza arthurgavazza requested a review from Jorropo March 23, 2023 17:22
@arthurgavazza arthurgavazza requested a review from Jorropo March 23, 2023 19:14
@arthurgavazza
Copy link
Copy Markdown
Contributor Author

@Jorropo sorry for asking again, can you take a look at this PR again please? If it's ok, what else do I need to do so we get this merged?

Copy link
Copy Markdown
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

Sorry for the delay.

LGTM two minor things and I'll merge

Thx a lot for your PR, this is great and saves us some time 🙂 ❤️

Co-authored-by: Jorropo <jorropo.pgm@gmail.com>
@Jorropo
Copy link
Copy Markdown
Contributor

Jorropo commented Mar 30, 2023

The linter is unhappy, that needs to be fixed 🙂 https://github.com/ipfs/kubo/actions/runs/4559382268/jobs/8043302197?pr=9745

@arthurgavazza
Copy link
Copy Markdown
Contributor Author

@Jorropo Is there a local command so I can verify any lint warnings? I fixed the warnings from the workflow.
Thank you for reviewing my code and helping me through this first PR ❤️

@arthurgavazza arthurgavazza requested a review from Jorropo March 30, 2023 01:55
@Jorropo
Copy link
Copy Markdown
Contributor

Jorropo commented Mar 30, 2023

@arthurgavazza I belive you can do:

go run github.com/golangci/golangci-lint/cmd/golangci-lint@latest ./...

At the root of the repo.

You can also run the individual linters in a similar fashion (golangci-lint is a tool that compiles and makes it easier to run a bunch of them).

Copy link
Copy Markdown
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

Thx LGTM

@Jorropo
Copy link
Copy Markdown
Contributor

Jorropo commented Mar 30, 2023

Noting here, I had of unfilled values and null when trying it out, that because I didn't passed it all the optin flags.
So I added omitempty to various fields:

type connInfo struct {
	Addr      string         `json:",omitempty"`
	Peer      string         `json:",omitempty"`
	Latency   string         `json:",omitempty"`
	Muxer     string         `json:",omitempty"`
	Direction inet.Direction `json:",omitempty"`
	Streams   []streamInfo   `json:",omitempty"`
	Identify  IdOutput       `json:",omitempty"`
}

@Jorropo Jorropo enabled auto-merge (squash) March 30, 2023 03:42
@Jorropo Jorropo merged commit e89cce6 into ipfs:master Mar 30, 2023
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