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

Adds Scalyr logging endpoint support #73

Merged
merged 1 commit into from
May 19, 2020
Merged

Adds Scalyr logging endpoint support #73

merged 1 commit into from
May 19, 2020

Conversation

mccurdyc
Copy link
Collaborator

Relevant (Referenced) Link(s)

Validation

$ make test
git clone git@github.com:mccurdyc/cli.git /tmp/cli && \
(
    cd /tmp/cli && \
    git checkout mccurdyc/logging-scalyr && \
    make test && \
    rm -rf /tmp/cli \
)

@mccurdyc mccurdyc requested a review from phamann May 14, 2020 15:40
@mccurdyc mccurdyc marked this pull request as ready for review May 14, 2020 15:50
@akappen
Copy link

akappen commented May 14, 2020

We just released region support for Scalyr logging. The API documentation has been updated if you'd like to include support for that in the CLI. Thanks!

@mccurdyc mccurdyc added the enhancement New feature or request label May 14, 2020
@pteichman
Copy link
Contributor

Conflict fix is over at e556630

@mccurdyc
Copy link
Collaborator Author

@akappen Awesome! We will have to include in fastly/go-fastly and then cut a new release and then update the dependency version used in the CLI and then update this PR.

@mccurdyc
Copy link
Collaborator Author

@phamann I'm going to come back to this to address this comment, but I would like your thoughts on the CreateCommand.createInput() Create*Input pattern as I will adopt this in the other open PRs and come back to Scalyr once I cut a new go-fastly release.

Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

👍 LGTM. Regardless of the small nits around certain input validations, I like the new crateInput() pattern overall and how you've tested it. I'm on the fence around the naming convention of scalyr_test.go. and api_test.go as this breaks convention from the rest of the project. What was your reasoning for keeping createInput() private and thus having this fork?

Comment on lines 66 to 63
if c.Version <= 0 {
return nil, fmt.Errorf("error invalid value for required field (version): %d", c.Version)
}

input.Version = c.Version
Copy link
Member

Choose a reason for hiding this comment

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

Whilst this is good, we don't validate negative numbers for version input anywhere else in the project. I'm also conscious of doing this kind of client-side validation when the API will do it anyway.

The big revelation for me here is why are the version fields in go-fastly uint's to avoid this completely 😱 (not your fault, just convention there I hadn't realised until now)

pkg/logging/scalyr/create.go Outdated Show resolved Hide resolved
pkg/logging/scalyr/create.go Show resolved Hide resolved
@mccurdyc
Copy link
Collaborator Author

Force-pushed rebasing master.

@mccurdyc
Copy link
Collaborator Author

Force-pushed squashing into a single commit.

Signed-off-by: Colton McCurdy <cmccurdy@fastly.com>
@mccurdyc
Copy link
Collaborator Author

Force-pushed rebasing master.

@mccurdyc mccurdyc merged commit 4b1d37a into fastly:master May 19, 2020
@mccurdyc mccurdyc deleted the mccurdyc/logging-scalyr branch May 19, 2020 18:41
@mccurdyc mccurdyc changed the title logging: adds Scalyr logging endpoint support Adds Scalyr logging endpoint support May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants