Skip to content
This repository was archived by the owner on Jan 19, 2018. It is now read-only.

Implement param pointers to Nulecule spec.#191

Open
cdrage wants to merge 3 commits into
projectatomic:masterfrom
cdrage:add-xpathing
Open

Implement param pointers to Nulecule spec.#191
cdrage wants to merge 3 commits into
projectatomic:masterfrom
cdrage:add-xpathing

Conversation

@cdrage

@cdrage cdrage commented Nov 27, 2015

Copy link
Copy Markdown
Member

This implements XPathing as per the Atomic App PR projectatomic/atomicapp#366.

Fixes issue #70.

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

Comment thread spec/README.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

its "Nulecule Specification"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, I should clarify that the RFC implements JSON pointers (inherentdly how we do xpathing)

@goern

goern commented Dec 1, 2015

Copy link
Copy Markdown
Contributor

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.

@cdrage cdrage force-pushed the add-xpathing branch 2 times, most recently from 770a657 to 2205ddf Compare December 1, 2015 15:55
Comment thread spec/param.json Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this file we're providing a programatic view of what the data structure looks like, not an example.

@aweiteka

aweiteka commented Dec 2, 2015

Copy link
Copy Markdown
Contributor

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

@cdrage

cdrage commented Dec 2, 2015

Copy link
Copy Markdown
Member Author

@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

cdrage commented Dec 17, 2015

Copy link
Copy Markdown
Member Author

ping @aweiteka @goern updated again

@cdrage

cdrage commented Dec 21, 2015

Copy link
Copy Markdown
Member Author

timeout... pinging again
@goern @aweiteka

and @vpavlin

@cdrage cdrage changed the title Implement XPathing to Nulecule spec. Implement param pointers to Nulecule spec. Dec 21, 2015
@aweiteka

Copy link
Copy Markdown
Contributor

@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] 7a83d58

@cdrage

cdrage commented Dec 22, 2015

Copy link
Copy Markdown
Member Author

@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 :)

@cdrage cdrage force-pushed the add-xpathing branch 3 times, most recently from 8d8d453 to 73513e1 Compare December 22, 2015 16:27
@cdrage cdrage force-pushed the add-xpathing branch 2 times, most recently from 3d7c120 to beef97f Compare December 22, 2015 16:28
@aweiteka

Copy link
Copy Markdown
Contributor
param:
  - /spec/containers/0/param

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

@aweiteka

Copy link
Copy Markdown
Contributor

@cdrage I submitted PR to update your branch.

Describe param as structured data
@cdrage

cdrage commented Jan 12, 2016

Copy link
Copy Markdown
Member Author

ping @aweiteka @bexelbie can we merge this?

@cdrage

cdrage commented Feb 11, 2016

Copy link
Copy Markdown
Member Author

timeout, pinging again @aweiteka @bexelbie

@goern

goern commented Feb 11, 2016

Copy link
Copy Markdown
Contributor

LGTM

@vpavlin ?

@bexelbie

Copy link
Copy Markdown

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

cdrage commented Mar 14, 2016

Copy link
Copy Markdown
Member Author

@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

@cdrage cdrage added the feature label Mar 14, 2016
@dustymabe

Copy link
Copy Markdown
Contributor

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

@dustymabe

Copy link
Copy Markdown
Contributor

ping @cdrage

@cdrage

cdrage commented Apr 7, 2016

Copy link
Copy Markdown
Member Author

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

cdrage commented Jun 6, 2016

Copy link
Copy Markdown
Member Author

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants