Implement param pointers to Nulecule spec.

projectatomic / nulecule

[UNMAINTAINED] Specification describing a container-based application

209 stars 46 forks source link

Implement param pointers to Nulecule spec. #191

Open cdrage opened 8 years ago

cdrage commented 8 years ago

This implements XPathing as per the Atomic App PR https://github.com/projectatomic/atomicapp/pull/366.

Fixes issue #70.

I'm not too good at modfying / changing spec information, so will need some input from @bexelbie and @aweiteka

goern commented 8 years ago

can you add a little why and what to the XPath chapter. Same for "XPathing", it seems to be called JSON patching, as in the RFC you mentioned.

aweiteka commented 8 years ago

Per atomicapp pr#366 shouldn't this be part of the artifacts object?

cdrage commented 8 years ago

@aweiteka @goern

Yes, it's suppose to be part of the artifacts object but I was unable to find a clean example to indicate it (the spec uses different definitions than our atomic app implementation).

~~Could one of you please take-over this issue and possibly create another PR to merge in? I'm not too good at modifying/improving specifications.~~ let me update and try again :)

cdrage commented 8 years ago

ping @aweiteka @goern updated again

cdrage commented 8 years ago

timeout... pinging again @goern @aweiteka

and @vpavlin

aweiteka commented 8 years ago

@cdrage I'm confused how this relates to this[1]. It seems like a duplication. I don't want to ask more of you so I think at this point I would like to take a pass at this. Would you mind?

[1] https://github.com/projectatomic/nulecule/commit/7a83d58c7ff34dbc00c94be252921d384e6092b0

cdrage commented 8 years ago

@aweiteka That would be my mistake, seems I had merged xpathing during the 0.3.0 release by mistake. I've since reverted that.

Feel free to check this PR again :)

aweiteka commented 8 years ago

param:
  - /spec/containers/0/param

I still can't reconcile this with atomicapp implementation https://github.com/projectatomic/atomicapp/pull/366 . I don't think it belongs in constraints. I also believe it is better described in artifacts, not params definition.

aweiteka commented 8 years ago

@cdrage I submitted PR to update your branch.

cdrage commented 8 years ago

ping @aweiteka @bexelbie can we merge this?

cdrage commented 8 years ago

timeout, pinging again @aweiteka @bexelbie

goern commented 8 years ago

LGTM

@vpavlin ?

bexelbie commented 8 years ago

I agree with @aweiteka that this belongs in the artifacts section or at least not in constraints.

If atomic app varies from the spec we should rationalize the two. Perhaps we need a working spec call?

cdrage commented 8 years ago

@bexelbie @goern @dustymabe

This is badddd documentation in my regards, this should have never been committed. I will update this.

I also vote to move more towards yaml instead since it gives a lot more clarity in regards to the spec (not only the xpathing examples).

This is what xpathing looks like within Atomic App and thus within artifacts:

artifacts:
   kubernetes:
       - resource: artifacts/kubernetes/template.json
         params:
            foo:
               - /spec/template/metadata/foo
            bar:
               - /spec/template/metadata/bar
      - resource: artifacts/kubernetes/extra.json

dustymabe commented 8 years ago

@cdrage.. can we get a TL;DR for this? I'm not sure I follow everything.

dustymabe commented 8 years ago

ping @cdrage

cdrage commented 8 years ago

TLDR:

If you wish to change a value within a json/yaml file, you can specify the value and position in the file to replace, ex:

            param1:
                - /spec/containers/0/ports/0/hostPort
                - /spec/containers/0/ports/0/hostPort2

cdrage commented 8 years ago

@goern @aweiteka

Once #209 is merged, I can add this to the spec as it'll be easier to understand from a usability perspective on how to use this.