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.

Sign up for GitHub

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

Jump to bottom

update forbidden messages #2319

Merged
merged 1 commit into from
May 21, 2015
Merged

update forbidden messages #2319

merged 1 commit into from
May 21, 2015

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented May 18, 2015

Fixes #1558.

Allows for forbidden messages on a verb/resource basis.

@liggitt

liggitt
liggitt reviewed May 18, 2015
View reviewed changes
pkg/authorization/authorizer/interfaces.go
MakeMessage(ctx MessageContext) (string, error)
}

// MessageContext contains sufficient information to create a forbidden message. It is bundled in this one object to make it easy and obvious hwo to build a golang template
Copy link
Contributor

Choose a reason for hiding this comment

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

s/hwo/how/

@liggitt liggitt self-assigned this May 18, 2015
smarterclayton
smarterclayton reviewed May 18, 2015
View reviewed changes
pkg/authorization/authorizer/messages.go
type ForbiddenMessageResolver struct {
// TODO if these maps were map[string]map[util.StringSet]ForbiddenMessageMaker, we'd be able to handle cases where sets of resources wanted slightly different messages
// unfortunately, maps don't support keys like that, requiring StringSet serialization and deserialization.
namespacedVerbsToResourcesToForbiddenMessageMaker map[string]map[string]ForbiddenMessageMaker
Copy link
Contributor

Choose a reason for hiding this comment

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

This wins an award for Longest Variable Name in a Go struct, 2015.

@deads2k deads2k force-pushed the forbidden-message branch from 6975716 to 79dd65a Compare May 19, 2015 13:32
@deads2k
Copy link
Contributor Author

deads2k commented May 19, 2015

Fixed typo.

@deads2k
Copy link
Contributor Author

deads2k commented May 20, 2015

@liggitt bump.

liggitt
liggitt reviewed May 20, 2015
View reviewed changes
pkg/authorization/authorizer/messages.go
}

func newTemplateForbiddenMessageMaker(text string) templateForbiddenMessageMaker {
parsedTemplate, err := template.New("").Parse(text)
Copy link
Contributor

Choose a reason for hiding this comment

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

template.Must(template.New(...

@liggitt
Copy link
Contributor

liggitt commented May 20, 2015

LGTM with a cursory reading

@deads2k deads2k force-pushed the forbidden-message branch from 79dd65a to c6a7597 Compare May 20, 2015 14:26
@deads2k
Copy link
Contributor Author

deads2k commented May 20, 2015

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2037/) (Image: devenv-fedora_1586)

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/2306/)

@deads2k deads2k force-pushed the forbidden-message branch from c6a7597 to 38dcdcc Compare May 20, 2015 18:47
@deads2k
Copy link
Contributor Author

deads2k commented May 20, 2015

re[merge]

@deads2k
Copy link
Contributor Author

deads2k commented May 20, 2015

flake:

fatal: Unable to create '/data/src/github.com/openshift/origin-bare/refs/heads/master.lock': File exists.

re[merge]

@deads2k deads2k force-pushed the forbidden-message branch from 38dcdcc to 9276301 Compare May 20, 2015 20:41
@deads2k
Copy link
Contributor Author

deads2k commented May 21, 2015

flake I haven't seen before: https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2031/

error: timed out waiting for the condition
!!! Error in hack/test-cmd.sh:550
  'osc delete all --all' exited with status 1
Call stack:
  1: hack/test-cmd.sh:550 main(...)
Exiting with status 1

It passed on the test run: https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/2306/

delete all: ok

@derekwaynecarr you seen this one before?

[merge] please?

@deads2k
Copy link
Contributor Author

deads2k commented May 21, 2015

UI failure: https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2036/

/data/src/github.com/openshift/origin/assets/node_modules/protractor/node_modules/selenium-webdriver/lib/atoms/error.js:113
  var template = new Error(this.message);
                 ^
UnknownError: Error communicating with the remote browser. It may have died.

This thing is cursed. Pretty please [merge]?

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 9276301

openshift-bot pushed a commit that referenced this pull request May 21, 2015
Merge pull request #2319 from deads2k/forbidden-message
9b28703 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 9b28703 into openshift:master May 21, 2015
@deads2k deads2k deleted the forbidden-message branch May 21, 2015 18:59
@enj enj mentioned this pull request Aug 22, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Message for forbidden is amusing and probably too generic
4 participants
@deads2k @liggitt @openshift-bot @smarterclayton