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

Jump to bottom

Add up alias for build, push, deploy #503

Merged
merged 2 commits into from Aug 17, 2018
Merged

Add up alias for build, push, deploy #503

merged 2 commits into from Aug 17, 2018

Conversation

johnmccabe
Copy link
Contributor

@johnmccabe johnmccabe commented Aug 16, 2018 •
edited

Description

Add up alias for build ➡︎ push ➡︎ deploy.

faas-up

faas-up-skip-push

This reuses all the flags/logic from the respective commands which are chained together, it adds a single local flag --push which is used to include the push step.

Alternatives to setting --push would be to do so for non-localhost/127.0.0.1 gateways perhaps. @alexellis wdyt?

This reuses all the flags/logic from the respective commands which are chained together, it adds a single local flag --skip-push which is used to exclude the push step.

Motivation and Context

How Has This Been Tested?

Built and tested against a local gateway with both a local and docker hub image push/skip-push.

No unit tests added as it just reuses the underlying commands.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@johnmccabe
Add up alias for build, push, deploy
6a22fda 
This reuses all the flags/logic from the respective commands which are
chained together, it adds a single local flag `--push` which is used to
include the push step.

Alternatives to setting `--push` would be to do so for
non-localhost/127.0.0.1 gateways perhaps.

Signed-off-by: John McCabe <john@johnmccabe.net>
@johnmccabe johnmccabe changed the title [WIP] Add up alias for build, push, deploy [WIP] Add "up" alias for "build ➡︎ push ➡︎ deploy" workflow Aug 16, 2018
@johnmccabe johnmccabe changed the title [WIP] Add "up" alias for "build ➡︎ push ➡︎ deploy" workflow [WIP] Add "up" alias for build, push, deploy Aug 16, 2018
@johnmccabe johnmccabe changed the title [WIP] Add "up" alias for build, push, deploy [WIP] Add up alias for build, push, deploy Aug 16, 2018
alexellis
alexellis approved these changes Aug 16, 2018
View reviewed changes
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Now how about --watch? :)

@alexellis
Copy link
Member

Let's just have faas-cli push refuse if there is no prefix on the image. I think that's a separate change though. cc @viveksyngh

viveksyngh
viveksyngh reviewed Aug 16, 2018
View reviewed changes
commands/up.go Outdated
@@ -0,0 +1,69 @@
// Copyright (c) Alex Ellis 2017. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use OpenFaaS Author(s) 2018 for copyright. One of the feedback from @alexellis on my previous PR :)

Copy link
Member

Choose a reason for hiding this comment

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

+1 for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, its a WIP pr was going to address this later - funnily enough I was looking for that header text when raising the PR and noticed this - https://github.com/openfaas/faas-cli/blob/master/commands/update_gitignore.go, you'll need to update that one as well :)

alexellis
alexellis requested changes Aug 16, 2018
View reviewed changes
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

To merge we need the header changing and also push should be on by default, off by optional flag.

@ivanayov
Copy link
Contributor

I was wondering wether it will break with the flags, f.e faas up --no-cache, where --no-cache is available for build, but not for push/deploy.

We did a test with @alexellis and the command runs as expected.

@johnmccabe
Add --skip-push and update header
2f539c8 
Signed-off-by: John McCabe <john@johnmccabe.net>
@burtonr
Copy link
Contributor

burtonr commented Aug 16, 2018

Awesome @ivanayov I was wondering the same thing, but haven't had time to test it out yet. Good to know!

@johnmccabe johnmccabe changed the title [WIP] Add up alias for build, push, deploy Add up alias for build, push, deploy Aug 16, 2018
alexellis
alexellis approved these changes Aug 17, 2018
View reviewed changes
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

LGTM!

@alexellis alexellis merged commit e34c172 into openfaas:master Aug 17, 2018
@alexellis
Copy link
Member

Derek set milestone: 0.7.0

@derek derek bot added this to the 0.7.0 milestone Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viveksyngh viveksyngh viveksyngh left review comments

@alexellis alexellis alexellis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@johnmccabe @alexellis @ivanayov @burtonr @viveksyngh