Skip to content
This repository has been archived by the owner on Jan 14, 2020. It is now read-only.

Small fixes #114

Merged
merged 3 commits into from Sep 27, 2018
Merged

Small fixes #114

merged 3 commits into from Sep 27, 2018

Conversation

ojkelly
Copy link
Contributor

@ojkelly ojkelly commented Sep 17, 2018

This PR has some fixes to minor regressions.

@coveralls
Copy link

coveralls commented Sep 17, 2018

Pull Request Test Coverage Report for Build 513

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 38.571%

Totals Coverage Status
Change from base Build 505: 0.0%
Covered Lines: 675
Relevant Lines: 1750

💛 - Coveralls

"",
)
printer.SubStep("Cleaning up unused change set", 1, true, true)
_, err := cf.DeleteChangeSet(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is causing a test failure, since the cfn wrapper is called client rather than cf.

)
}
printer.Stop()
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please not call os.Exit(1) directly? It complicates testing. Use printer.Fatal, or refactor the function to return an error, and let it cascade back out of the testable path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good pickup, this was a quick fix, not ideal to merge in like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

After double checking, I've noticed that there's more os.Exit calls scattered around the place. I think I'll make a pass at trying to refactor those out.

@ojkelly ojkelly merged commit f227fc3 into master Sep 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants