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

Remove ACR builder #1308

Merged
merged 5 commits into from
Nov 22, 2018
Merged

Remove ACR builder #1308

merged 5 commits into from
Nov 22, 2018

Conversation

balopat
Copy link
Contributor

@balopat balopat commented Nov 22, 2018

Should only be merged after: #1305.

Fixes #1283.

@codecov-io
Copy link

codecov-io commented Nov 22, 2018

Codecov Report

Merging #1308 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1308      +/-   ##
==========================================
+ Coverage   44.85%   44.98%   +0.12%     
==========================================
  Files         106      106              
  Lines        4735     4737       +2     
==========================================
+ Hits         2124     2131       +7     
+ Misses       2389     2385       -4     
+ Partials      222      221       -1
Impacted Files Coverage Δ
pkg/skaffold/runner/runner.go 50.2% <ø> (+0.61%) ⬆️
pkg/skaffold/schema/v1alpha5/upgrade.go 64.7% <100%> (+6.08%) ⬆️
pkg/skaffold/schema/v1alpha5/defaults.go 49.01% <0%> (+1.96%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09e67c1...bff7897. Read the comment docs.

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Nov 22, 2018
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Nov 22, 2018
@dgageot dgageot merged commit a52cc32 into GoogleContainerTools:master Nov 22, 2018
@ehotinger
Copy link

@balopat @dgageot could one of you cc me when the plugin model is complete so we can add this back in? Or if you need ideas wrt the plugin model?

@SteveLasker
Copy link

I was happy to see ACR Tasks supported, but surprised to see if removed. Shouldn't the extension model have been figured out before removing a capability? We're happy to help support this so this is truly a cross cloud solution.

@nkubala
Copy link
Contributor

nkubala commented Mar 21, 2019

Shouldn't the extension model have been figured out before removing a capability?

@SteveLasker possibly, but our thinking was that we wanted to avoid setting the precedent and leading contributors to send us more new builders, which we'd have to reject even though this one was already accepted.

the builder plugin model has been implemented in skaffold for a few releases now, so we're happy to accept contributions for new builders as plugins! we'll have some guidelines for contributing coming pretty soon, but feel free to start poking around in the code before that :)

EDIT: I got a little ahead of myself here, so I wanted to clarify. what I should have said is that the plugin model has been initially implemented, but it's still somewhat experimental and we're still iterating on the design, so in its current state it's not exactly the friendliest model to work with for contributors. we're working on both fully converting our existing builders to plugins themselves and improving the design, as well as making it as easy to integrate with as possible, so definitely stay tuned to future releases for updates on it! sorry for any confusion I might have caused :)

@balopat
Copy link
Contributor Author

balopat commented Mar 21, 2019

@SteveLasker - thank you for your comment!

In terms of the ACR tasks - we did not have the capacity, or the infrastructure to support it with a clear conscience ourselves - thank you for offering to support it - this sounds like an interesting collaboration. I wonder what you had in mind for that?

At this point, I see the plugin model to be of a more future proof and cleaner direction than reverting this change back into the codebase.

We are very actively working on the plugin model and it is impacting the builder structure heavily - I would say that it is not in a shape yet to contribute to - I would estimate another couple of weeks before it gets into a good state.

@SteveLasker
Copy link

Fair enough. As the plug-in model evolves, we're happy to support the scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants