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

Add CurrentDropletGUID attribute to application resource [main] #3354

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

Conversation

gmllt
Copy link
Member

@gmllt gmllt commented Jan 8, 2025

Where this PR should be backported?

Description of the Change

The V3 API now allows you to access to the relationships existing beween an app and its current droplet.
This PR make this relationship visible trough a new CurrentDropletGUID attribute in the resource application.

The community cf_exporter currently use the /v2/spaces/:guid/summary endpoint to gain access to buildpack associated to an application. Have this relationship visible would help get rid of calls on this endpoint and retrieve informations from bbs about DesiredLRPs and RunningLRPS.

This feature will not cause breaking change.

Copy link
Contributor

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
I added some suggestions on naming, but I would like to see some tests to make sure that we are covering this change. They can be in unit tests in api/cloudcontroller/ccv3/relationship_test.go

@@ -42,10 +44,18 @@ func (a Application) MarshalJSON() ([]byte, error) {
Metadata: a.Metadata,
}

relationShips := Relationships{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
relationShips := Relationships{}
relationships := Relationships{}

ccApp.Relationships = Relationships{
constant.RelationshipTypeSpace: Relationship{GUID: a.SpaceGUID},
}
relationShips[constant.RelationshipTypeSpace] = Relationship{GUID: a.SpaceGUID}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
relationShips[constant.RelationshipTypeSpace] = Relationship{GUID: a.SpaceGUID}
relationships[constant.RelationshipTypeSpace] = Relationship{GUID: a.SpaceGUID}

}

if a.CurrentDropletGUID != "" {
relationShips[constant.RelationshipTypeCurrentDroplet] = Relationship{GUID: a.CurrentDropletGUID}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
relationShips[constant.RelationshipTypeCurrentDroplet] = Relationship{GUID: a.CurrentDropletGUID}
relationships[constant.RelationshipTypeCurrentDroplet] = Relationship{GUID: a.CurrentDropletGUID}

relationShips[constant.RelationshipTypeCurrentDroplet] = Relationship{GUID: a.CurrentDropletGUID}
}

if len(relationShips) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this if statement here? if we could I would prefer to have it removed and always set relationships in the struct

}

if len(relationShips) > 0 {
ccApp.Relationships = relationShips
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ccApp.Relationships = relationShips
ccApp.Relationships = relationships

@joaopapereira
Copy link
Contributor

I updated the current PR with the latest changes in main to rerun the tests and see if everything passes. I did not check the changes in the v8 PR, but I assume the same modifications would apply so if you have some time, please make the same changes there.

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