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

Add in delete mixin command #632

Merged
merged 8 commits into from Sep 20, 2019
Merged

Add in delete mixin command #632

merged 8 commits into from Sep 20, 2019

Conversation

bradcypert
Copy link
Contributor

@bradcypert bradcypert commented Sep 19, 2019

What does this change

This PR adds in the ability to delete a mixin by using the porter mixin delete NAME command, where NAME is a mixin name like "helm".

➜ porter mixin delete
Error: no mixin name was specified

➜ porter mixin delete helm
Error: Could not find helm in the mixin directory.

➜ porter mixin install helm
installed helm mixin
helm v0.8.0-beta.1 (8325d71) by DeisLabs

➜ porter mixin delete helm
Deleted helm mixin

What issue does it fix

Closes #613

Notes for the reviewer

This is my first PR and I tried to follow as many existing patterns and practices as I could, but if something seems off, just let me know and I'll try to update it :)

Checklist

  • Unit Tests
  • Documentation
    • Documentation Not Impacted

@msftclas
Copy link

msftclas commented Sep 19, 2019

CLA assistant check
All CLA requirements met.

@bradcypert
Copy link
Contributor Author

@carolynvs if you have a moment, can you checkout this branch and see if builds work locally for you? I haven't done anything with dep before, and I'm not 100% sure it's setup properly. I'm getting errors when trying to build, but it looks like CI is fine?

➜ make build
go generate ./...
/Library/Developer/CommandLineTools/usr/bin/make --no-print-directory build MIXIN=porter -f mixin.mk BINDIR=bin
mkdir -p bin
go build -ldflags '-w -X github.com/deislabs/porter/pkg.Version=v0.16.1-beta.1-2-gcee6ef7 -X github.com/deislabs/porter/pkg.Commit=cee6ef7' -o bin/porter ./cmd/porter
# github.com/dieslabs/porter/cmd/porter
cmd/porter/mixins.go:77:10: undefined: mixin.DeleteOptions
cmd/porter/mixins.go:86:12: p.DeleteMixin undefined (type *porter.Porter has no field or method DeleteMixin)
make[1]: *** [build-client] Error 2
make: *** [build-porter] Error 2

@carolynvs
Copy link
Member

Oh! Not all of the CI runs for new contributors until we have reviewed and said it's safe to run. I'll make sure to mention that in the contributing guide.

Here let me kick it off for you quick so you can see the full CI run in azure pipelines.

@carolynvs
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@bradcypert
Copy link
Contributor Author

@carolynvs disregard. I found my mistake. dieslabs => deislabs.

@carolynvs
Copy link
Member

I just left a comment on why I think the build is failing for you. Try moving that struct and hopefully it should pass (or at least move on to new and more interesting errors!). 🤞

@carolynvs
Copy link
Member

Oh boy, I will never be good at helping people find spelling mistakes! 😂

@bradcypert bradcypert marked this pull request as ready for review September 19, 2019 19:36
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

This is looking great! Just a few small changes need to be made to get it ready to merge.

Gopkg.lock Outdated Show resolved Hide resolved
pkg/mixin/mixin.go Outdated Show resolved Hide resolved
pkg/mixin/delete.go Show resolved Hide resolved
pkg/mixin/delete.go Outdated Show resolved Hide resolved
pkg/mixin/provider/delete.go Outdated Show resolved Hide resolved
pkg/mixin/provider/delete.go Outdated Show resolved Hide resolved
pkg/mixin/provider/delete_test.go Outdated Show resolved Hide resolved
pkg/porter/mixins.go Show resolved Hide resolved
@carolynvs
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@bradcypert
Copy link
Contributor Author

@carolynvs I made some changes around the areas that you mentioned above. If anything else looks off, please don't hesitate to lemme know. I'm just thrilled to have the opportunity to learn from a large go project :)

@carolynvs
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@carolynvs
Copy link
Member

I'm going to fix our CI so that the bulk of it runs automatically for external contributors. Thanks for you patience with us having to run them manually for the next few days! 😊

CONTRIBUTING.md Outdated
* [What is the general code layout?](#what-is-the-general-code-layout)
* [Logging](#logging)

- [How to help](#how-to-help)
Copy link
Member

Choose a reason for hiding this comment

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

It helps to not include unrelated changes in a pull request. Fixing a single bad link is fine to sneak in though, and it sounds like from your commit message that was the intent. Let's revert the changes to this file other than the missing ) and this should be ready to merge. 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that! I wanted to fix the missing ) and I think my markdown plugin ran a format-on-save. I should have checked the diff before committing it! I'll get this fixed.

@carolynvs
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

This looks great, thank you! 👍

@carolynvs carolynvs merged commit a010a15 into getporter:master Sep 20, 2019
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.

Support deleting a mixin with the porter mixin delete command
3 participants