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

remove unknown args and added tests #211

Merged
merged 1 commit into from
Oct 26, 2016
Merged

remove unknown args and added tests #211

merged 1 commit into from
Oct 26, 2016

Conversation

procrypt
Copy link

@procrypt procrypt force-pushed the unknown_args branch 2 times, most recently from 486d2f6 to d4f95ad Compare October 17, 2016 09:50
@dustymabe
Copy link
Contributor

can you rebase now that the rename has happened?

@procrypt
Copy link
Author

@dustymabe rebase done.

@dustymabe
Copy link
Contributor

@procrypt - can you "sign the cla"? basically create a linuxfoundation account with your redhat email address and then visit this link and "sign up as an employee".

dustymabe
dustymabe reviewed Oct 17, 2016
View reviewed changes
cli/app/app.go
@@ -253,3 +256,10 @@ func askForConfirmation() bool {
return askForConfirmation()
}
}

func unknownArgs(c *cli.Context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

my preference would be to rename this function to TestForUnknownArgs or CheckForUnknownArgs

Copy link
Author

Choose a reason for hiding this comment

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

@dustymabe sure i'll rename the function.

@procrypt procrypt force-pushed the unknown_args branch 2 times, most recently from 7e9cb9a to 9045024 Compare October 17, 2016 19:51
@kadel
Copy link
Member

kadel commented Oct 18, 2016 •
edited
Loading

Just one thing. Have you consider putting that check to validateFlags instead of adding it to every command?
validateFlags is already doing some validation and it is already called in every command.

I'm not saying that you should change it like this. I'm just interested why you did it like this. Valid answer can be that you think that it makes more sense the way you did it. 😉

I tested your code and it works great. 👍

@procrypt
Copy link
Author

@kadel I didn't know about the function validateFlags we have for doing the validation thing, so I went ahead and wrote a new function which does the validation :)
I can update the PR if you want to use validateFlags instead of the CheckForUnknownArgs.

@kadel
Copy link
Member

kadel commented Oct 19, 2016

I think that would be better to have all the validation on one place ;-)

@procrypt
Copy link
Author

@kadel I'll update the PR :)

@kadel
Copy link
Member

kadel commented Oct 19, 2016

Oh and one more think. It looks like your commit has some weird Author.
Author: Abhishek <abhishek@dhcp35-217.lab.eng.blr.redhat.com> so even if you already signed CLA it will still fail because it cannot associate this commit with your github account.

@procrypt
Copy link
Author

I'll check and make sure it will be correct this time.

@procrypt procrypt force-pushed the unknown_args branch 2 times, most recently from 5df25ce to 5ef3fcc Compare October 19, 2016 13:51
sebgoa
sebgoa reviewed Oct 20, 2016
View reviewed changes
script/test/cmd/tests.sh
@@ -58,4 +58,10 @@ convert::expect_success "kompose -f $KOMPOSE_ROOT/script/test/fixtures/volume-mo
# openshift test
convert::expect_success "kompose --provider=openshift -f $KOMPOSE_ROOT/script/test/fixtures/volume-mounts/simple-vol-mounts/docker-compose.yml convert --stdout --dc" "$KOMPOSE_ROOT/script/test/fixtures/volume-mounts/simple-vol-mounts/output-os.json"

######
# Tests related to unknown arguments with cli commands
convert::expect_failure "kompose up docker $KOMPOSE_ROOT/script/test/fixtures/unknown-arguments/docker-gitlab.yml" "Unknown Argument docker-gitlab.yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

'kompose up docker' I am not sure what that is ? Why do you add docker in that line ?

Copy link
Author

Choose a reason for hiding this comment

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

@sebgoa This might added by mistake, will make the changes.

cli/app/app.go
@@ -203,6 +205,7 @@ func Up(c *cli.Context) {

// Down deletes all deployment, svc.
func Down(c *cli.Context) {
CheckForUnknownArgs(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want to update this function and actually use ValidateFlags as mentioned by @kadel

Copy link
Author

Choose a reason for hiding this comment

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

@sebgoa I'm working on this will update the PR.

@dustymabe dustymabe mentioned this pull request Oct 20, 2016
Closed
@procrypt procrypt force-pushed the unknown_args branch 11 times, most recently from 61a817d to d9d74c6 Compare October 20, 2016 22:16
@procrypt
Copy link
Author

@kadel @sebgoa I have update the PR, can you please review it.

@procrypt procrypt force-pushed the unknown_args branch 8 times, most recently from c0a09b5 to 685e26d Compare October 21, 2016 06:34
kadel
kadel approved these changes Oct 21, 2016
View reviewed changes
Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @procrypt

@kadel kadel added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2016
@procrypt
Copy link
Author

@kadel :)

@dustymabe
Copy link
Contributor

looks like merge conflict - need to resolve it.

@pradeepto
Copy link

Is this merge able now? cc @dustymabe @procrypt @kadel @ngtuna

@kadel
Copy link
Member

kadel commented Oct 26, 2016

@pradeepto Yes it is.
Some time ago we established workflow where everyone is merging his/her own PR after LGTM, so I left this for @procrypt to merge it himself.

@procrypt
Copy link
Author

@kadel I was not aware of that sorry my bad.

@kadel
Copy link
Member

kadel commented Oct 26, 2016

Ah, no don't worry.
We should probably talk about this and document PR review process, and change it, because this cant' work for someone who doesn't have write access to repo.

@kadel kadel merged commit 414645d into kubernetes:master Oct 26, 2016
@kadel
Copy link
Member

kadel commented Oct 26, 2016

Ah, no don't worry.
We should probably talk about this and document PR review process.

On Wed, Oct 26, 2016 at 2:25 PM, Abhishek Pratap Singh <
notifications@github.com> wrote:

@kadel https://github.com/kadel I was not aware of that sorry my bad.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#211 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADfdnLb7DojF9MP4w1rc9Mx_5hR-z_jks5q30amgaJpZM4KYaxb
.

@procrypt procrypt deleted the unknown_args branch November 3, 2016 09:23
@surajssd surajssd mentioned this pull request Nov 11, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dustymabe dustymabe dustymabe left review comments

@sebgoa sebgoa sebgoa left review comments

@kadel kadel kadel approved these changes

Assignees
No one assigned
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@procrypt @dustymabe @kadel @pradeepto @sebgoa