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

Control plane - get and update app services manifest endpoints #126

Merged
merged 10 commits into from
Mar 1, 2022

Conversation

alonmr
Copy link
Contributor

@alonmr alonmr commented Feb 28, 2022

No description provided.

@alonmr
Copy link
Contributor Author

alonmr commented Feb 28, 2022

The methods getResource https://github.com/v3io/v3io-go/blob/development/pkg/controlplane/http/session.go#L624
and getResourceDetail https://github.com/v3io/v3io-go/blob/development/pkg/controlplane/http/session.go#L463 looks almost identical, we should probably remove one of them in the future.

Copy link
Contributor

@liranbg liranbg left a comment

Choose a reason for hiding this comment

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

💪🏼 minor comments

Comment on lines 396 to 404
err := s.listResource(getAppServicesManifestInput.Ctx,
"app_services_manifests",
&getAppServicesManifestInput.ControlPlaneInput,
&getAppServicesManifestOutput.ControlPlaneOutput,
&getAppServicesManifestOutput.AppServicesManifests)

if err != nil {
return nil, err
}
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
err := s.listResource(getAppServicesManifestInput.Ctx,
"app_services_manifests",
&getAppServicesManifestInput.ControlPlaneInput,
&getAppServicesManifestOutput.ControlPlaneOutput,
&getAppServicesManifestOutput.AppServicesManifests)
if err != nil {
return nil, err
}
if err := s.listResource(getAppServicesManifestInput.Ctx,
"app_services_manifests",
&getAppServicesManifestInput.ControlPlaneInput,
&getAppServicesManifestOutput.ControlPlaneOutput,
&getAppServicesManifestOutput.AppServicesManifests); err != nil {
return nil, errors.Wrap(err, "Failed to list app services manifests")
}

getJobOutput := v3ioc.GetJobOutput{}

// try to update the resource
err := s.updateResource(updateAppServicesManifestInput.Ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment above with if err := ...

}

if err := json.NewDecoder(responseBuffer).Decode(&jsonAPIResponse); err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

wrap err ^

&getAppServicesManifestInput.ControlPlaneInput,
&getAppServicesManifestOutput.ControlPlaneOutput,
&getAppServicesManifestOutput.AppServicesManifests); err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

wrap err

&updateAppServicesManifestInput.AppServicesManifest,
&getJobOutput.ControlPlaneOutput,
&getJobOutput.JobAttributes); err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

wrap err

@liranbg liranbg merged commit ea23e24 into v3io:development Mar 1, 2022
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