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

Disable dab #653

Merged
merged 1 commit into from
Jun 21, 2017
Merged

Disable dab #653

merged 1 commit into from
Jun 21, 2017

Conversation

cdrage
Copy link
Member

@cdrage cdrage commented Jun 19, 2017

See issue: #390

Remove DAB as it is hard to maintain / not much usage / DAB is still
experimental in Docker and there hasn't been much movement:
moby/moby#26876

MarkDeprecated does not work at the moment due to issue:
#652

However, that is not a blocker as we fatalF within ValidateFlags

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 19, 2017
@cdrage
Copy link
Member Author

cdrage commented Jun 19, 2017

dabbing

ping @sebgoa @kadel

@sebgoa
Copy link
Contributor

sebgoa commented Jun 19, 2017

so +1 , except that as you mention MarkDeprecated does not work.

@cdrage
Copy link
Member Author

cdrage commented Jun 19, 2017

@sebgoa Yes, an issue with Cobra, which already has a fix, see: spf13/cobra#466 will just have to wait until it's merged upstream. But since we error out anyways if --bundle is passed, this will still work. We'll just have to update vendoring once spf13/cobra#466 is merged 👍

@cdrage cdrage changed the title Remove dab Disable dab Jun 19, 2017
@cdrage
Copy link
Member Author

cdrage commented Jun 19, 2017

I've also updated the commit title / information.

We're disabling DAB not removing it, as the code for conversion is still there for future use!

See issue: kubernetes#390

Disable DAB as it is hard to maintain / not much usage / DAB is still
experimental in Docker and there hasn't been much movement:
moby/moby#26876

MarkDeprecated does not work at the moment due to issue:
kubernetes#652

However, that is not a blocker as we `fatalF` within `ValidateFlags`
@cdrage
Copy link
Member Author

cdrage commented Jun 20, 2017

Okay. Tests pass now. @kadel @surajssd want to do a quick review?

@surajnarwade
Copy link
Contributor

works for me :)

@procrypt
Copy link
Contributor

procrypt commented Jun 21, 2017

@cdrage LGTM and +1 for disabling DAB 💃

@procrypt
Copy link
Contributor

@cdrage Do we need to write the functional tests for dab in golang since we are disabling it.

@pradeepto
Copy link

@cdrage Do we need to write the functional tests for dab in golang since we are disabling it.

No, we definitely shouldn't but I will let @cdrage and others take the final call.

@cdrage
Copy link
Member Author

cdrage commented Jun 21, 2017

@procrypt @surajnarwade Remeber to go through the review process and hit "approve" 👍 But since I got two confirmations, let's go ahead and merge this!

@cdrage cdrage merged commit cf39f78 into kubernetes:master Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants