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.

Sign up for GitHub

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

Jump to bottom

Adds SFTP logging endpoint support #77

Merged
merged 1 commit into from
May 20, 2020
Merged

Adds SFTP logging endpoint support #77

merged 1 commit into from
May 20, 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-sftp && \
    make test && \
    rm -rf /tmp/cli \
)

@mccurdyc mccurdyc requested a review from phamann May 14, 2020 19:52
mccurdyc
mccurdyc commented May 14, 2020
View reviewed changes
pkg/logging/sftp/create.go Outdated Show resolved Hide resolved
mccurdyc
mccurdyc commented May 14, 2020
View reviewed changes
pkg/logging/sftp/update.go Outdated Show resolved Hide resolved
@mccurdyc mccurdyc marked this pull request as ready for review May 14, 2020 20:01
@mccurdyc mccurdyc added the enhancement New feature or request label May 14, 2020
@pteichman
Copy link
Contributor

Conflict fix is at c0afb13

@mccurdyc
Copy link
Collaborator Author

Force-pushed Peter's commit.

@mccurdyc
Copy link
Collaborator Author

Force-pushed rebasing master.

@mccurdyc
Copy link
Collaborator Author

Force-pushed squashing into single commit.

@mccurdyc
Copy link
Collaborator Author

mccurdyc commented May 19, 2020 •
edited
Loading

Force-pushed a squashed commit that fixed the failed linting into a single commit.

ezkl
ezkl suggested changes May 20, 2020
View reviewed changes
Copy link
Contributor

@ezkl ezkl left a comment

Choose a reason for hiding this comment

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

I found a few issues and made comments, but it looks great otherwise. The Uint and String helpers are only called for optional fields when their corresponding flag is set. It might be worthwhile to use the NullString helper for strings instead?

pkg/logging/sftp/create.go Show resolved Hide resolved
pkg/logging/sftp/create.go Show resolved Hide resolved
pkg/app/run_test.go

-s, --service-id=SERVICE-ID Service ID
--version=VERSION Number of service version
-d, --name=NAME The name of the SFTP logging object
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like -n would be more appropriate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ezkl I agree with you. However, using -d was adopted from a standard set by s3 for the describe subcommand.

While it may not be a big deal, this change would be backward incompatible --- unless we added -n --- for anyone using the cli in scripts, etc.

I'm assuming there was a reason for explicitly using -d for describe cc @phamann.

Copy link
Member

Choose a reason for hiding this comment

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

Haha very good catch, I think that was a copy/pasta error in the S3 implementation and we should use -n. I'm also happy with the breaking change to S3, which I can follow up with in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@phamann I can make this change across all logging endpoints after we get them all merged in.

pkg/logging/sftp/sftp_test.go Outdated Show resolved Hide resolved
pkg/logging/sftp/sftp_test.go Outdated Show resolved Hide resolved
pkg/logging/sftp/sftp_test.go Show resolved Hide resolved
pkg/logging/sftp/sftp_test.go Show resolved Hide resolved
@mccurdyc
Copy link
Collaborator Author

Force-pushed squash into single commit to include --port as required for creates.

@mccurdyc mccurdyc requested a review from ezkl May 20, 2020 13:02
@mccurdyc
Copy link
Collaborator Author

Force-pushed squashed commit making port option.

phamann
phamann approved these changes May 20, 2020
View reviewed changes
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 other than the comments we've discussed.

ezkl
ezkl approved these changes May 20, 2020
View reviewed changes
Copy link
Contributor

@ezkl ezkl left a comment

Choose a reason for hiding this comment

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

good work!

@mccurdyc
logging: adds SFTP logging endpoint support
3c96d5e 
Signed-off-by: Colton McCurdy <cmccurdy@fastly.com>
@mccurdyc
Copy link
Collaborator Author

Force-pushed rebasing master.

@mccurdyc mccurdyc merged commit 264c782 into fastly:master May 20, 2020
@mccurdyc mccurdyc deleted the mccurdyc/logging-sftp branch May 20, 2020 18:45
@mccurdyc mccurdyc changed the title logging: adds SFTP logging endpoint support Adds SFTP logging endpoint support May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phamann phamann phamann approved these changes

@ezkl ezkl ezkl approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mccurdyc @pteichman @ezkl @phamann