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

Make github credentials compulsory to avoid bad deployments #284

Merged
merged 6 commits into from Jul 1, 2015
Merged

Conversation

campoy
Copy link
Contributor

@campoy campoy commented Jun 30, 2015

Last time I deployed I forgot to set the github credentials causing random failures (403).

I don't want this happening again.

if cred.ClientID == "" || cred.ClientSecret == "" {
log.Fatal("secret.json needs to define ClientID and ClientSecret")
}
return fmt.Sprintf("client_id=%s&client_secret=%s", cred.ClientID, cred.ClientSecret)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the values need to be escaped via url.QueryEscape, or are they guaranteed to never use any symbols that might need escaping?

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 should not make a difference if the value is escaped or not

Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably use url.Values and then its Encode method to create this string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dmitshur
Copy link
Contributor

I think this PR should remove the config.go.template if it's no longer needed.

@campoy
Copy link
Contributor Author

campoy commented Jun 30, 2015

PTAL

@dmitshur
Copy link
Contributor

LGTM.

@@ -33,7 +33,7 @@ func init() {
}

var (
contactEmail = "golang-dev@googlegroups.com"
contactEmail = "unknown@example.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed by mistake - removed now

adg added a commit that referenced this pull request Jul 1, 2015
Make github credentials compulsory to avoid bad deployments
@adg adg merged commit 8dcf5fb into master Jul 1, 2015
@dmitshur dmitshur deleted the secrets branch March 16, 2016 04:43
@gopherbot gopherbot restored the secrets branch March 16, 2016 04:44
@adg adg deleted the secrets branch March 16, 2016 05:22
@gopherbot gopherbot restored the secrets branch March 16, 2016 05:24
@adg adg deleted the secrets branch March 16, 2016 05:29
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