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.

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

Add types for cross project quota #9029

Merged
merged 2 commits into from
Jun 16, 2016
Merged

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented May 25, 2016

Adds the types for cross project quota in a new API group.

@openshift/api-review

@deads2k
Copy link
Contributor Author

deads2k commented May 25, 2016

@derekwaynecarr ptal

// this resource.
Selector map[string]string

// Spec defines the desired quota
Copy link
Contributor

Choose a reason for hiding this comment

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

Quota

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2016
@deads2k
Copy link
Contributor Author

deads2k commented Jun 6, 2016

I need to check fuzzer tests. It seems unlikely that they all just fell out correctly.


// orderedMap is a very simple ordering a map tracking insertion order. It allows fast and stable serializations
// for our encoding. You could probably do something fancier with pointers to interfaces, but I didn't.
type orderedMap struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarterclayton not the most efficient structure, but I think its good enough to start.

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2016
@deads2k
Copy link
Contributor Author

deads2k commented Jun 7, 2016

Reminder. It would be nice to have this ready when I got back :)

}

// ResourceQuotasStatusByNamespace provides type correct methods
type ResourceQuotasStatusByNamespace struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not a type alias?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this not a type alias?

Not all OrderedMaps can be dealt with as ResourceQuotaStatusByNamespaces. I don't want to encourage casting.

@deads2k
Copy link
Contributor Author

deads2k commented Jun 13, 2016

Any issues in concept? List versus slice is internal enough change later.

// ByNamespace slices the usage by namespace. This division allows for quick resolution of
// deletion reconcilation inside of a single namespace without requiring a recalculation
// across all namespaces. This can be used to pull the deltas for a given namespace.
ByNamespace ResourceQuotasStatusByNamespace `json:"byNamespace"`
Copy link
Contributor

Choose a reason for hiding this comment

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

NamespaceStatuses, consistent with ContainerStatuses. by is awkward. Rename internally as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or Namespaces. I think I like Namespaces.

@smarterclayton
Copy link
Contributor

Are you using the new label selector?

@deads2k
Copy link
Contributor Author

deads2k commented Jun 13, 2016

Are you using the new label selector?

Yes. Wasn't expecting it in unversioned though. Selector *unversioned.LabelSelector, right?

@smarterclayton
Copy link
Contributor

Guess so. API is fine if you change to Namespaces instead of ByNamespaces

@deads2k
Copy link
Contributor Author

deads2k commented Jun 13, 2016

Name changed. [test]

@deads2k deads2k force-pushed the cq-01 branch 3 times, most recently from 062f903 to 01a93f7 Compare June 15, 2016 18:26
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2016
@deads2k
Copy link
Contributor Author

deads2k commented Jun 16, 2016

[merge]

@deads2k deads2k force-pushed the cq-01 branch 2 times, most recently from aee06d9 to 4a57da1 Compare June 16, 2016 13:12
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2016
@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 16, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4994/) (Image: devenv-rhel7_4391)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 51a1e57

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 51a1e57

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4988/)

@openshift-bot openshift-bot merged commit 24ae73e into openshift:master Jun 16, 2016
@@ -0,0 +1,3 @@
// Package v1 is the v1 version of the API.
// +genconversion=true
Copy link
Contributor

Choose a reason for hiding this comment

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

@mfojtik mind fixing this in your clientset generation PR? You're putting that comment in types.go rather than package doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

@soltysh clientset generator works differently, it needs to be in types to get the name of the type...

Copy link
Contributor

Choose a reason for hiding this comment

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

/me was blind, I blame Monday ;)

@deads2k deads2k deleted the cq-01 branch September 6, 2016 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants