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

bug(create team): Update status on the Team CRD in the admin namespace #2469

Merged
merged 1 commit into from Dec 10, 2018

Conversation

Projects
None yet
4 participants
@markawm
Copy link
Member

markawm commented Dec 10, 2018

Submitter checklist

  • Change is code complete and matches issue description.
  • Change is covered by existing or new tests.

Description

The code tries to use the admin namespace as returned by kube.GetAdminNamespace(), but this appears to just return
the current namespace rather than finding the admin namespace - i.e. it can change!
Fixed by being explicit at all times about the admin NS to use. But I think there is another todo to sort out the current
'TODO' in kube.GetAdminNamespace():
// TODO find the admin namespace via a label on the current dev namespace - or use current?

Special notes for the reviewer(s)

Which issue this PR fixes

fixes #

bug(create team): Update status on the Team CRD in the admin (not tea…
…m) namespace.

The code tries to use the admin namespace as returned by kube.GetAdminNamespace(), but this appears to just return
the current namespace rather than finding the admin namespace - i.e. it can change!
Fixed by being explicit at all times about the admin NS to use. But I think there is another todo to sort out the current
'TODO' in kube.GetAdminNamespace().

@jenkins-x-bot jenkins-x-bot requested review from rawlingsj and wbrefvem Dec 10, 2018

@markawm

This comment has been minimized.

Copy link
Member

markawm commented Dec 10, 2018

@@ -161,7 +161,8 @@ func (o *DeleteTeamOptions) deleteTeam(name string) error {
}
o.changeNamespace(name)

err = o.ModifyTeam(name, func(team *v1.Team) error {
//TODO: will be wrong admin ns here.

This comment has been minimized.

@jtnord

jtnord Dec 10, 2018

Member

ahh... #2451 ?

This comment has been minimized.

@markawm

markawm Dec 10, 2018

Member

i'm just turning my attention to the delete one now - but yes, I suspect that is the cause of wrecking the admin namespace!

@jtnord
Copy link
Member

jtnord left a comment

Looks ok, but maybe a review by @jstrachan here.

@markawm

This comment has been minimized.

Copy link
Member

markawm commented Dec 10, 2018

/retest

@markawm

This comment has been minimized.

Copy link
Member

markawm commented Dec 10, 2018

Created task to follow-up with James S re. kube.GetAdminNamespace behaviour as a possible better fix for this.
Wasn't able to get the build logs for the first failure; tests pass locally, and /retest was clean.

@jtnord
Copy link
Member

jtnord left a comment

/lgtm

@jenkins-x-bot jenkins-x-bot added the lgtm label Dec 10, 2018

@jenkins-x-bot

This comment has been minimized.

Copy link
Contributor

jenkins-x-bot commented Dec 10, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jtnord

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jenkins-x-bot jenkins-x-bot merged commit 5834d50 into jenkins-x:master Dec 10, 2018

1 of 2 checks passed

tide Not mergeable.
Details
serverless-jenkins succeeded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment