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

ccv3.Client : replace usages of newHTTPRequet when possible #6

Conversation

gmllt
Copy link

@gmllt gmllt commented Jun 23, 2023

Description of the Change

This PR replaces usages of ccv3.Client newHTTPRequest method in favor of ccv3.Client MakeRequest, MakeListRequest and MakeRequestUploadAsync methods for the following ccv3.Client methods :

  • CreateBuildpack
  • DeleteBuildpack
  • GetBuildpacks
  • UpdateBuildpack
  • UploadBuildpack
  • uploadBuildpackAsynchronously
  • GetApplicationEnvironment
  • GetFeatureFlag
  • GetFeatureFlags
  • UpdateFeatureFlag
  • GetApplicationManifest
  • EntitleIsolationSegmentToOrganizations
  • ShareServiceInstanceToSpaces
  • ResourceMatch
  • CreateApplicationTask
  • GetApplicationTasks
  • UpdateTaskCancel

Why Is This PR Valuable?

This PR standardize interactions with the ccv3.Client, using MakeRequest, MakeListRequest and MakeRequestUploadAsync for the methods mentionned above as they are already used in others ccv3.Client methods.

How Urgent Is The Change?

Those changes are not urgent but will help to get rid of a failure encountered during the entitlement of an IsolationSegment to an Organization using the cloudfoundry terraform provider (cloudfoundry-community/terraform-provider-cloudfoundry#449).

@Thanhphan1147
Copy link

Hello, the PR looks ok to me. However I have a slight doubt since this means that we are diverging further from the client implementation at the upstream (cloudfoundry/cli). I think simply copying the implementations from the v7 or v8 branch would be a better solution. What do you think?

@gmllt gmllt force-pushed the ccv3-implementations-uses-ccv3-client-make-requests branch from 61c25f1 to f827354 Compare August 1, 2023 13:45
@gmllt gmllt requested a review from sleungcy August 1, 2023 13:46
@gmllt
Copy link
Author

gmllt commented Aug 1, 2023

@Thanhphan1147 I have updated the branch. I have copied implementations from the upstream, and extracted ressources as well.
Is this what you were thinking off ?

@Thanhphan1147
Copy link

Hello, looks good to me. Thanks

@gmllt
Copy link
Author

gmllt commented Sep 10, 2023

@sleungcy, can you take a look at this PR again (i modified it to include @Thanhphan1147's recommendations) ?

@gmllt
Copy link
Author

gmllt commented Dec 15, 2023

@sleungcy, can you take a look at this PR again (i modified it to include @Thanhphan1147's recommandations) ?


return responseBuildpack, response.Warnings, err
return responseBody, warnings, err

Choose a reason for hiding this comment

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

actually wouldn't responseBody be undefined here?

Copy link
Author

Choose a reason for hiding this comment

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

My bad, thanks for noticing it.
It should be ok now

Copy link

@sleungcy-sap sleungcy-sap left a comment

Choose a reason for hiding this comment

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

seeing one issue

@gmllt
Copy link
Author

gmllt commented Dec 20, 2023

I've also removed unnecessary definitions for ccv3.APILink, ccv3.APILinks and ccv3.Metadata.
I've updated ccv3 tests accordingly to modifications in ccv3.client.
I don't if updating the v7action ccclient was necessary, so i isolated it in a single commit (in case it's not).

@sleungcy-sap
Copy link

@loafoe can you help have a look?

@gmllt
Copy link
Author

gmllt commented Jan 29, 2024

@loafoe can you take a look at this PR ?
We need those changes so we can upgrade the cloudfoundry terraform provider we use to deploy.
Currently, we are locked to v0.15.4.

@loafoe
Copy link
Member

loafoe commented Jan 29, 2024

@gmllt if @sleungcy and @Thanhphan1147 are OK I'm fine merging this one.

Copy link

@sleungcy sleungcy left a comment

Choose a reason for hiding this comment

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

lgtm, ran a subset of the provder's tests locally with no issues.

@loafoe loafoe merged commit cda5ed8 into cloudfoundry-community:v0.0.11-complete-api Jan 30, 2024
loafoe pushed a commit to cloudfoundry-community/terraform-provider-cloudfoundry that referenced this pull request Jan 30, 2024
…9a5+incompatible (#549)

This version of cloudfoundry-cli fixes bugs occuring when using some specifics
ressources such as isolation segments.

(See [ccv3.Client : replace usages of newHTTPRequet when possible #6](cloudfoundry-community/cloudfoundry-cli#6))
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.

5 participants