-
Notifications
You must be signed in to change notification settings - Fork 388
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
Finish eliminating plugin usage in 'doctl sls functions' + testing improvements #1252
Finish eliminating plugin usage in 'doctl sls functions' + testing improvements #1252
Conversation
This will stay as a draft until PR #1232 is merged since that PR has higher priority. Once 1232 is merged, this PR will be rebased and marked for review. |
9d8bb61
to
c7e7be5
Compare
This completes the elimination of plugin usage in doctl sls fn and the functions.go source file.
c7e7be5
to
8a26142
Compare
I believe this happened in merge conflict resolution during the recent rebase.
return []whisk.Action{}, err | ||
} | ||
if limit == 0 { | ||
limit = 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the default before the changeover. Maintaining it to avoid surprising new behaviors.
Affects what happens when a failure occurs in the middle of deleting functions and triggers together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM. I did have one question in line, but it is not specific to this PR.
nameSort, _ := c.Doit.GetBool(c.NS, flagNameSort) | ||
nameName, _ := c.Doit.GetBool(c.NS, flagNameName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not something new to this PR, but I wonder if we should hide one of these flags as they have the same purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Apache OpenWhisk CLI (wsk
) just uses --name-sort
, short form -n
. When Adobe issued their CLI (aio
), it appears that they introduced the "alias" flag --name
and they moved the -n
shortcut to it. By itself, this flag is indeed kind of confusing (seems like it should take a value and allow you to specify a name, somehow).
When I incorporated parts of aio
into nim
I retained the alias behavior. When I created the doctl
delegation path I again retained the behavior. In this PR, in recoding the logic in golang
I again retained the behavior (copying is easier than thinking). I think it's appropriate to go back to what wsk
does, which retains --name-sort
and -n
but not --name
. This is technically a breaking change but I think it's extremely unlikely to matter to anyone.
@andrewsomething do you agree with that plan? If so, I'm inclined to just do it (by tacking the change onto this PR). What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ach. Since it is a behavior change, and technically breaking, I decided not to do it as part of this PR so I'm proceeding to merge. We can think about making the proposed simplification in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Sounds good.
This change covers internal improvements not intended to be visible to typical
doctl
users.doctl sls fn list
doctl sls status
doctl sls connect
permitting connection to serverless clusters that are not in production (and hence unknown to the serverless portal). This is needed to support internal testing and debugging use cases for which we currently use thenim
CLI (which should be phased out).