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

Enable deployment settings in source control #15945

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

glena
Copy link
Contributor

@glena glena commented Apr 15, 2024

Description

Fixes # (issue)

Checklist

  • I have run make tidy to update any new dependencies
  • I have run make lint to verify my code passes the lint check
    • I have formatted my code using gofumpt
  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

@pulumi-bot
Copy link
Contributor

Changelog

[uncommitted] (2024-04-15)

@glena glena marked this pull request as ready for review April 16, 2024 16:25
@glena glena requested a review from a team as a code owner April 16, 2024 16:25
Copy link
Collaborator

@tgummerer tgummerer left a comment

Choose a reason for hiding this comment

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

I think this PR could benefit from a bit more description/a linked issue that describes these new commands in a little more detail, e.g. what their intended behaviour is, and the intended user experience.

deployment apitype.DeploymentSettings,
) error {
// The local backend does not currently persist tags.
return errors.New("stack deployments not supported in --local mode")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return errors.New("stack deployments not supported in --local mode")
return errors.New("stack deployments not supported with diy backends")

Similar below, as someone might not be using --local, but azure blob or s3 or similar backends, where --local would be somewhat confusing.

func (pc *Client) UpdateStackDeployment(ctx context.Context, stack StackIdentifier,
deployment apitype.DeploymentSettings,
) error {
// TODO(german): validate deployment
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this worth doing now? Otherwise can we write down which validations we expect to do here? I guess the backend will also do its own validation anyway, so we would be getting an error here if the deployment isn't valid?

Copy link
Member

Choose a reason for hiding this comment

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

We should probably remove the TODO. No need for us to validate CLI side of the service can do it better.

}

func (pc *Client) DestroyStackDeployment(ctx context.Context, stack StackIdentifier) error {
// TODO(german): validate deployment
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like we have a deployment to validate here? Is this TODO accurate?

}

func (pc *Client) GetStackDeployment(ctx context.Context, stack StackIdentifier) (*apitype.DeploymentSettings, error) {
// TODO(german): validate deployment
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to be validating the response? Something else?


func newDeploymentsCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "deployments",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Use: "deployments",
Use: "deployment",

Should we be using the singular form here, since each invocation will only affect a single deployment afaics?

@glena glena marked this pull request as draft April 17, 2024 11:43
@glena
Copy link
Contributor Author

glena commented Apr 17, 2024

@tgummerer @Frassle sry, this was meant to be a draft PR. Started as a PoC and is not ready for review. Thank you for the feedback I will reach out again once it is ready. I might have mixed up with this one #15946 when I flagged it as ready

@tgummerer
Copy link
Collaborator

No worries, happy to review when it is ready!

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.

None yet

4 participants