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

cmd,daemon: changes in api data to show components information #14097

Conversation

alfonsosanchezbeato
Copy link
Member

Changes to start send and processing components data on installation. With this we can show better messages on installation of a local component.

if only components are going to be installed.
@github-actions github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jun 18, 2024
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

did a first quick pass, a code org suggestion

Comment on lines +353 to +354
apiData["components"] = map[string][]string{
instanceName: {compInfo.Component.ComponentName},
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is the same format that we use as input in the new APIs right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is correct

cmd/snap/cmd_snap_op.go Show resolved Hide resolved
Copy link
Member Author

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

@pedronis thanks, I've addressed your points now

cmd/snap/cmd_snap_op.go Show resolved Hide resolved
Comment on lines +353 to +354
apiData["components"] = map[string][]string{
instanceName: {compInfo.Component.ComponentName},
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is correct

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

nitpicks

cmd/snap/cmd_snap_op.go Outdated Show resolved Hide resolved
cmd/snap/cmd_snap_op.go Outdated Show resolved Hide resolved
Copy link
Member

@andrewphelpsj andrewphelpsj left a comment

Choose a reason for hiding this comment

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

Thank you, just one tiny nitpick.

// components.
func (csd *changedSnapsData) changedSnaps() (names []string, notOnlyComps map[string]bool) {
names = make([]string, 0, len(csd.comps)+len(csd.names))
notOnlyComps = map[string]bool{}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
notOnlyComps = map[string]bool{}
notOnlyComps = make(map[string]bool, len(csd.names))

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@alfonsosanchezbeato
Copy link
Member Author

Addressed last comments and squashed commits now

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thanks

@alfonsosanchezbeato alfonsosanchezbeato merged commit fb27fbc into canonical:master Jun 23, 2024
44 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Documentation -auto- Label automatically added which indicates the change needs documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants