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

fixed preapply commands and added postapply/postplan #102

Merged
merged 2 commits into from
Aug 10, 2017
Merged

fixed preapply commands and added postapply/postplan #102

merged 2 commits into from
Aug 10, 2017

Conversation

mootpt
Copy link
Contributor

@mootpt mootpt commented Aug 5, 2017

Per #100

  • Fixed issue with preapply commands not being executed
  • Added post commands
  • Updated docs and examples

@CLAassistant
Copy link

CLAassistant commented Aug 5, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@anubhavmishra anubhavmishra left a comment

Choose a reason for hiding this comment

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

@lkysow could you also have a look?

@@ -0,0 +1,89 @@
// Package postrun handles running commands after the
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a lot of redundant code here with prerun package doing the same thing. We should restructure this so we can use the code for both prerun and postrun pacakges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. I'll get rid of the copy paste replace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anubhavmishra The code is basically the same, I could move duplicate functions with different names in a more general package "run" or we could just rename the package and functions to "run". What were you thinking?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep! run might make more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay @anubhavmishra , settled on a more general package and opted to pass in the stage name(i.e "pre_plan", "post_plan", etc)

@@ -178,6 +181,14 @@ func (a *ApplyExecutor) apply(ctx *CommandContext, repoDir string, plan models.P
}
ctx.Log.Info("apply succeeded")

// if there are post apply commands then run them
if len(config.PostApply.Commands) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are adding post_apply might as well add post_plan too. Some people might want to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing. for whatever reason i just figure pre_apply was the same thing as post_plan, but you are right, the steps are different. Let me add it.

@mootpt
Copy link
Contributor Author

mootpt commented Aug 6, 2017

looks like e2e test failed. code seems fine. seems like environment variable doesn't exist in circle?

@anubhavmishra
Copy link
Collaborator

yep. we might need to change it so that end to end test only run on master builds.

@mootpt mootpt changed the title fixed preapply commands and added postapply fixed preapply commands and added postapply/postplan Aug 6, 2017
@anubhavmishra
Copy link
Collaborator

anubhavmishra commented Aug 7, 2017

#108 should fix the e2e failures for now. We will come up with a better plan to figure out when and how to run e2e tests. So you can rebase when the PR is merged.

@anubhavmishra
Copy link
Collaborator

@mootpt #108 is merged. you can rebase now.

@anubhavmishra anubhavmishra merged commit 6f583aa into hootsuite:master Aug 10, 2017
@mootpt
Copy link
Contributor Author

mootpt commented Aug 11, 2017

Sorry i missed this update. Thanks for the merge!

Sent from my Samsung SM-G935T using FastHub

@anubhavmishra
Copy link
Collaborator

All good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants