Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

pkg: add an acirenderer library. #464

Closed
wants to merge 1 commit into from
Closed

Conversation

sgotti
Copy link
Contributor

@sgotti sgotti commented Feb 4, 2015

This is the split of #297 in two parts. This first patch is the pure acirendering without any dependency resolving. It can be commited alone without any previous PR.
I hope it should also be useful for appc/docker2aci#6

This library renders an aci given a well defined list of dependencies.
A future patch will also provide the dependency resolving logic.

It adds two interfaces: ACIProvider and ACIRegistry.

The first interface is the one used by this patch and the cas has been changed
to satisfy it.
The second interface will be used by the dependency resolution functions. Also
this interface is satisfied by the cas.

@sgotti sgotti force-pushed the acirendererbase branch 2 times, most recently from 0e3ef95 to b422fc7 Compare February 4, 2015 15:25
@sgotti
Copy link
Contributor Author

sgotti commented Feb 4, 2015

With the feedback from docker2aci I rewrote it with a better algorithm so now it will create a list of files to extract for every aci.

This list can be used by another function to directly create a tar (like needed by docker2aci, or to extract it to a filesystem, like the original acirenderer).

The extraction part is in its file (extract.go) so this can also be splitted and the acirenderer core, if needed, can be moved to another place.

PTAL.

"github.com/appc/spec/schema/types"
)

// An ACIRegistry provides al functions of an ACIProvider plus functions to
Copy link
Member

Choose a reason for hiding this comment

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

al -> all


// ACIFiles represent which files to extract for every ACI
type ACIFiles struct {
key string
Copy link
Member

Choose a reason for hiding this comment

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

I need these exported if I want to access them later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Done!

@sgotti sgotti force-pushed the acirendererbase branch 4 times, most recently from f0189b7 to f66d327 Compare February 5, 2015 15:34
@sgotti
Copy link
Contributor Author

sgotti commented Feb 5, 2015

@iaguis @jonboulle Thanks a lot for your review! I tried to fix all the comments (let me know if someone was left behind). In the meantime I added an additional test with a 3 depth dependencies tree.

@sgotti sgotti force-pushed the acirendererbase branch 2 times, most recently from 555d108 to 7bed0b3 Compare February 5, 2015 17:25
@sgotti
Copy link
Contributor Author

sgotti commented Feb 5, 2015

@iaguis @jonboulle In the meantime I fixed an error in getUpperPwlm, added a test and docs for it.

@sgotti
Copy link
Contributor Author

sgotti commented Feb 9, 2015

@jonboulle @iaguis if you're ok I'd like to PR the acirenderer core to appc/spec (or appc/tools or whatever you prefer) as suggested in appc/docker2aci#10 and open another PR for the rocket part.

@iaguis
Copy link
Member

iaguis commented Feb 10, 2015

That would be great but I'm not sure where it should go. I'm leaning towards something like appc/tools since this is only one implementation of rendering and things in the spec should be more general. @jonboulle?

@sgotti sgotti force-pushed the acirendererbase branch 4 times, most recently from a7c33cc to 35bc3fb Compare February 16, 2015 08:59
@sgotti sgotti mentioned this pull request Feb 16, 2015
This library renders an aci given a well defined list of dependencies.
A future patch will also provide the dependency resolving logic.

It adds two interfaces: ACIProvider and ACIRegistry.

The first interface is the one used by this patch and the cas has been changed
to satisfy it.
The second interface will be used by the dependency resolution functions. Also
this interface is satisfied by the cas.
sgotti added a commit to sgotti/acido that referenced this pull request Feb 16, 2015
Use a copy of acirenderer as proposed in rkt/rkt#464 and
rkt/rkt#465.
@iaguis
Copy link
Member

iaguis commented Feb 19, 2015

@jonboulle suggested OOB that since it's not that much code, we could put acirenderer in appc/spec/actool in anticipation of splitting it out into a different repo soon.

I'm not sure if it makes sense conceptually to put it there but it would work and I'd like to move forward with this.

So #465 would have to be moved to appc/spec/actool too but #533 and #322 would stay in rocket, along with the remaining DB stuff (#393 , #394, #395) and the tree cache (#332).

What do you guys think about it?

@sgotti
Copy link
Contributor Author

sgotti commented Feb 20, 2015

@jonboulle Talking on irc with @iaguis we thought about proposing this and #465 to appc/spec/pkg/acirenderer. What do you think about it? (in the meantime I'll prepare the PRs 😄 )

@jonboulle
Copy link
Contributor

Seems reasonable, thanks!

On Fri, Feb 20, 2015 at 6:31 AM, Simone Gotti notifications@github.com
wrote:

@jonboulle https://github.com/jonboulle Talking on irc with @iaguis
https://github.com/iaguis we thought about proposing this and #465
#465 to appc/spec/pkg/acirenderer.
What do you think about it? (in the meantime I'll prepare the PRs [image:
😄] )


Reply to this email directly or view it on GitHub
#464 (comment).

@jonboulle
Copy link
Contributor

@sgotti so this one can be closed, right? and we land #533 instead?

@iaguis
Copy link
Member

iaguis commented Mar 9, 2015

Yes, appc/spec#216 superseded this

@jonboulle
Copy link
Contributor

thanks @iaguis

@jonboulle jonboulle closed this Mar 9, 2015
@sgotti
Copy link
Contributor Author

sgotti commented Mar 9, 2015

Sorry, I forgot to close this!

@sgotti sgotti deleted the acirendererbase branch May 24, 2015 16:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants