Skip to content
This repository has been archived by the owner on Mar 24, 2022. It is now read-only.

Formatting and style updates, closes #43 #58

Merged
merged 13 commits into from
Nov 21, 2018
Merged

Formatting and style updates, closes #43 #58

merged 13 commits into from
Nov 21, 2018

Conversation

mattysweeps
Copy link
Contributor

@mattysweeps mattysweeps commented Nov 20, 2018 •
edited
Loading

This PR performs code style updates to improve the Go Report Card.
https://goreportcard.com/report/github.com/cf-platform-eng/gcp-pcf-quickstart#golint

Works towards #43

TravisCI is also added, to check tests and run golint

@mattysweeps
code style changes
5085a97
@mattysweeps
remove old omg-cli binary when deploying
811f13d 
This ensures new executions of the deploy_pcf.sh script use the latest
code. This is useful when testing new changes.
@mattysweeps
Lint fixes
f6f1c5b 
Mostly capitalizing acronyms, few other changes.
@mattysweeps
More lint fixes.
f023624
@mattysweeps
Run go fmt
e8cdab1
@mattysweeps
reorder imports
ea7b7d6
@mattysweeps
format a few errors
aa5e1ff
@mattysweeps
Add a travisCI build file
611fb50
@mattysweeps
Copy link
Contributor Author

@rkoster , @evandbrown

Since I do not have admin on this project, I cannot turn on TravisCI support. (free, just link with travisci.org)

TravisCI should be default ensure PRs to master both pass lint checks and unit tests.

If anyone has disagreements, let me know your thought. I can always revert the TravisCI commit.

josephlewis42
josephlewis42 reviewed Nov 20, 2018
View reviewed changes
src/omg-cli/omg/setup/project.go Outdated
@@ -38,7 +38,7 @@ type QuotaError struct {
Region string
}

var UnsatisfiedQuotaErr = errors.New("Compute Engine quota is unsatisfied, request an increase at: https://console.cloud.google.com/iam-admin/quotas")
var ErrUnsatisfiedQuota = errors.New("unsatisfied quota for Compute Engine, request an increase at: https://console.cloud.google.com/iam-admin/quotas")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think Compute Engine should be in this string since the quotas are for project resources--even if they're technically part of the "compute engine" APIs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@mattysweeps
Copy link
Contributor Author

Reviewers:

All standard golint warning are fixed except for the fact that packages shouldn't have underscores in them. See https://blog.golang.org/package-names

I am going to wait till this PR is approved before I make any package renames.

Please let me know if package renaming is an issue. (It's very bad for downstream users, but I wouldn't think anyone is consuming this code as a dependency. I'd rather fix this once and never have to worry again)

The renames will be:

gcp_director -> director
service_broker -> servicebroker
stackdriver_nozzle -> strackdrivernozzle
ops_manager -> opsman

Let me know if you think the packages should be renamed differently.

rkoster
rkoster approved these changes Nov 21, 2018
View reviewed changes
src/omg-cli/omg/commands/cleanup_project.go

errsMu.Lock()
errors = append(errors, err)
errsMu.Unlock()
}
wg.Done()
}()
}(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work, this should fix and issue I was running into today (cleanup not deleting the ops-man director, which would then start recreating the just deleted instances).

src/omg-cli/omg/tiles/ert/resource.go
},
"light-bosh-stemcell-97.32-google-kvm-ubuntu-xenial-go_agent",
StemcellName: "light-bosh-stemcell-97.32-google-kvm-ubuntu-xenial-go_agent",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be updated in the output of pivnet_meta.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rkoster I opened an issue to fix that, see #59

@mattysweeps
Copy link
Contributor Author

I'm going to merge and perform the package renaming in a second PR

@mattysweeps mattysweeps merged commit 7ddfbd4 into master Nov 21, 2018
@mattysweeps mattysweeps deleted the go-report-card branch November 28, 2018 18:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@josephlewis42 josephlewis42 josephlewis42 left review comments

@rkoster rkoster rkoster approved these changes

@evandbrown evandbrown Awaiting requested review from evandbrown

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

Successfully merging this pull request may close these issues.
3 participants
@mattysweeps @rkoster @josephlewis42