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

Re-writes activations list command subtree in Go #1277

Merged

Conversation

ddebarros
Copy link
Contributor

@ddebarros ddebarros commented Oct 11, 2022

This PR re-write the doctl sls activations list subtree in native go-lang. All existing behavior is maintained.

@joshuaauerbachwatson
Copy link
Contributor

@ddebarros I am seeing commits in this PR that were already merged in the earlier PR. Did this get properly rebased?

@ddebarros
Copy link
Contributor Author

@ddebarros I am seeing commits in this PR that were already merged in the earlier PR. Did this get properly rebased?

I rebased it again. Should be all good now.

Copy link
Contributor

@joshuaauerbachwatson joshuaauerbachwatson left a comment

Choose a reason for hiding this comment

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

Excellent job so far @ddebarros . I left some comments which should probably be addressed but I think the main issue at this point is that the tests don't pass yet.

commands/activations.go Outdated Show resolved Hide resolved
commands/displayers/activations.go Outdated Show resolved Hide resolved
commands/serverless_util.go Outdated Show resolved Hide resolved
@ddebarros ddebarros marked this pull request as ready for review October 12, 2022 18:39
Copy link
Contributor

@joshuaauerbachwatson joshuaauerbachwatson left a comment

Choose a reason for hiding this comment

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

I just noticed that this PR includes a change to a vendor-supplied package. I would think we should not be doing that (certainly not lightly). I misunderstood in earlier conversations. I think we need to understand why this change is necessary and can the goal be accomplished differently.

commands/activations.go Show resolved Hide resolved
@ddebarros ddebarros changed the title Add activations list Re-writes activations list command subtree in Go Oct 13, 2022
@ddebarros
Copy link
Contributor Author

I just noticed that this PR includes a change to a vendor-supplied package. I would think we should not be doing that (certainly not lightly). I misunderstood in earlier conversations. I think we need to understand why this change is necessary and can the goal be accomplished differently.

@joshuaauerbachwatson I created a PR on the openwhisk-client and updated our go.mod file to reflect this change.

Copy link
Contributor

@joshuaauerbachwatson joshuaauerbachwatson left a comment

Choose a reason for hiding this comment

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

Excellent. I approve.
Over to @andrewsomething for permission to merge.

Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

Looks great! A did have a few tiny nitpicks inline.

commands/activations.go Show resolved Hide resolved
commands/displayers/activations.go Outdated Show resolved Hide resolved
commands/displayers/activations.go Outdated Show resolved Hide resolved
commands/displayers/activations.go Outdated Show resolved Hide resolved
Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

@ddebarros ddebarros merged commit ca6267a into digitalocean:feature/serverless Oct 14, 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.

3 participants