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 Site related subcommands #2

Merged
merged 9 commits into from
Jul 14, 2023
Merged

Add Site related subcommands #2

merged 9 commits into from
Jul 14, 2023

Conversation

gugahoi
Copy link
Contributor

@gugahoi gugahoi commented Jul 12, 2023

This PR adds 3 new commands:

  • sites list
  • sites get [id]
  • sites publish [id]
  • sites domains [id]

All related to managing Webflow sites. It also adds a "failed request" check that I somehow overlooked.

Nevvulo
Nevvulo previously approved these changes Jul 13, 2023
Copy link

@Nevvulo Nevvulo left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, just left some minor comments

cmd/sites.go Show resolved Hide resolved
cmd/sites.go Outdated Show resolved Hide resolved
cmd/webhooks.go Show resolved Hide resolved
@gugahoi gugahoi requested a review from Nevvulo July 13, 2023 05:25
log.Fatalf("Request failed: %v", err)
}

var response PublishSiteResponse

Choose a reason for hiding this comment

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

It'd be nice to print the response result to make it clear that the cmd finished successfully

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The response is "queued": true which is not particularly helpful in my opinion so thought silence was better.

Choose a reason for hiding this comment

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

Oh ok, yea that's not a big deal. Alternatively I was thinking of some generic message that says: "Published successfully"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it might not have tho. The command was sent successfully and as per unix philosophy I think if there is no additional information, then the exit code should be enough to indicate if the action was successful.

Copy link

@anonimitoraf anonimitoraf Jul 14, 2023

Choose a reason for hiding this comment

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

Ah ok, queued != published. Fair enough

Copy link

@anonimitoraf anonimitoraf left a comment

Choose a reason for hiding this comment

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

LGTM - I like the fact that commands targeting the same resource are all together

@gugahoi gugahoi merged commit 944cbce into master Jul 14, 2023
@gugahoi gugahoi deleted the feature/list-sites branch July 14, 2023 03:48
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.

3 participants