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

Replace log.Fatal by return errors in deploy.go #3116

Merged
merged 6 commits into from
Jun 1, 2018

Conversation

CecileRobertMichon
Copy link
Contributor

What this PR does / why we need it: Return errors instead of doing log.Fatal inside functions

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

If applicable:

  • documentation
  • unit tests
  • tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

CecileRobertMichon added 2 commits June 1, 2018 10:40
@@ -159,20 +159,21 @@ func (dc *deployCmd) load(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to get client: %s", err.Error())
}

// autofillApimodel calls log.Fatal() directly and does not return errors
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not anymore 😈


err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
Copy link
Member

Choose a reason for hiding this comment

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

This is the last line of defense? (i.e., no return err?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a unit test

Copy link
Member

Choose a reason for hiding this comment

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

/me takes off beer goggles

@jackfrancis
Copy link
Member

/approve
/lgtm

@acs-bot acs-bot removed the lgtm label Jun 1, 2018
@codecov
Copy link

codecov bot commented Jun 1, 2018

Codecov Report

Merging #3116 into master will decrease coverage by <.01%.
The diff coverage is 15.38%.

@@            Coverage Diff             @@
##           master    #3116      +/-   ##
==========================================
- Coverage   51.89%   51.89%   -0.01%     
==========================================
  Files          99       99              
  Lines       15162    15164       +2     
==========================================
+ Hits         7869     7870       +1     
- Misses       6571     6572       +1     
  Partials      722      722


// cleanup, since auto-populations creates dirs and saves the SSH private key that it might create
defer os.RemoveAll(deployCmd.outputDirectory)

cs, _, err = validateApimodel(apiloader, cs, ver)
if err != nil {
log.Fatalf("unexpected error validating apimodel after populating defaults: %s", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jackfrancis you actually just made me realize this should be t not log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I changed it

Copy link
Member

Choose a reason for hiding this comment

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

O.K. beer googles worked!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🍻

@jackfrancis
Copy link
Member

/approve
/lgtm

@acs-bot acs-bot added the lgtm label Jun 1, 2018
@acs-bot
Copy link

acs-bot commented Jun 1, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@CecileRobertMichon CecileRobertMichon merged commit f5c08ef into Azure:master Jun 1, 2018
@ghost ghost removed the in progress label Jun 1, 2018
@CecileRobertMichon CecileRobertMichon deleted the refactor-deploy branch June 6, 2018 19:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants