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

restapi: implement export for origin resources #8570

Closed
wants to merge 1 commit into from
Closed

restapi: implement export for origin resources #8570

wants to merge 1 commit into from

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented Apr 20, 2016 •
edited
Loading

@0xmichalis
Copy link
Contributor Author

[test]

@0xmichalis
Copy link
Contributor Author

cc: @soltysh @mfojtik for the image and build parts respectively

soltysh
soltysh reviewed Apr 20, 2016
View reviewed changes
pkg/api/helpers.go
@@ -52,3 +54,19 @@ func GetFieldLabelConversionFunc(supportedLabels map[string]string, overrideLabe
return "", "", fmt.Errorf("field label not supported: %s", label)
}
}

// TODO: Move this upstream
func ExportObjectMeta(objMeta *kapi.ObjectMeta, exact bool) {
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 like this method modifies object in place. I'd rather prefer this returns modified object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally if exact is passed I'd leave the Name or GenerateName where applicable and Namespace. All other fields are not important. At least that's what I usually do when doing oc get -o yaml and modifying by hand.

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 like this method modifies object in place. I'd rather prefer this returns modified object.

Modifying in place is the whole pattern of RESTExportStrategy. It would be a little strange to not do it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you've just copied the method from export.go as is, still since this will be part of public API I'd make it return the modified object.

Copy link
Contributor

Choose a reason for hiding this comment

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

@deads2k convinced me ;)

@soltysh
Copy link
Contributor

soltysh commented Apr 21, 2016

LGTM
re-[test]

0xmichalis
0xmichalis reviewed Apr 21, 2016
View reviewed changes
pkg/build/registry/buildconfig/strategy.go
@@ -39,6 +40,7 @@ func (strategy) AllowUnconditionalUpdate() bool {
// PrepareForCreate clears fields that are not allowed to be set by end users on creation.
func (strategy) PrepareForCreate(obj runtime.Object) {
bc := obj.(*api.BuildConfig)
bc.Status.LastVersion = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should normalize the status object and not just latestVersion

@0xmichalis
Copy link
Contributor Author

We may want to sync between dc.status.latestVersion and bc.status.lastVersion

cc: @bparees @mfojtik @smarterclayton @ironcladlou

@bparees
Copy link
Contributor

bparees commented Apr 21, 2016

@Kargakis sync what about them exactly? are you suggesting a name change?

@0xmichalis
Copy link
Contributor Author

Yes, I assumed that they had the same name and run a negative test for oc get bc/ruby-sample-build --template='{{.status.latestVersion}}' expecting that it will not be populated with <no value>. And of course the test failed (got back <no value>) because there is no latestVersion for bcs. I should have double-checked the bc field name and I am not advocating for the name, but we may want to consider them being in sync in the next version.

@bparees
Copy link
Contributor

bparees commented Apr 21, 2016

i don't really like the bc.lastVersion field name anyway, but what i'd like to change it to is more like buildNumber or something.

@0xmichalis
Copy link
Contributor Author

Upstream they are using revision (albeit, as an annotation) for deployments and we also recently changed LATEST (which stands for latestVersion) to REVISION for oc get dc.

@0xmichalis
Copy link
Contributor Author

#8039 and #8444 flakes

[test]

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2016
smarterclayton
smarterclayton reviewed Apr 27, 2016
View reviewed changes
pkg/build/registry/build/strategy.go
if len(build.Status.Phase) == 0 {
build.Status.Phase = api.BuildPhaseNew
}
build.Status.Duration = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't you clearing the status the same way here as elsewhere (assigning BuildStatus{})?

@smarterclayton
Copy link
Contributor

A few comments

0xmichalis
0xmichalis reviewed Apr 27, 2016
View reviewed changes
pkg/build/registry/build/strategy.go
if exact {
return nil
}
if build.Status.Config != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I am wondering if this can ever be non-nil

@0xmichalis
Copy link
Contributor Author

Let's hold onto this until kubernetes/kubernetes#24855 is resolved upstream.

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2016
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2016
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2016
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2016
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to e5e3e2f

@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
@openshift-bot
Copy link
Contributor

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

@openshift-bot
Copy link
Contributor

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2016
@0xmichalis 0xmichalis closed this Jun 13, 2016
@deads2k deads2k mentioned this pull request Aug 1, 2016
Closed
@0xmichalis 0xmichalis deleted the export-on-the-server branch October 21, 2016 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
blocked component/restapi needs-api-review needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@0xmichalis @soltysh @bparees @smarterclayton @openshift-bot @mfojtik @miminar @deads2k