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

Implement param pointers to Nulecule spec. #191

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cdrage
Copy link
Member

@cdrage cdrage commented Nov 27, 2015

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

@@ -186,6 +186,37 @@ Field Name | Type | Description
<a name="parametersDefault"></a>default | `string` | **Optional** An optional default value for the parameter.
<a name="parametersHidden"></a>hidden | `string` | **Optional** An optional boolean signifying the parameter should be obscured when displayed.

##### XPathing
XPathing is supported by the Nulecule spec as by [RFC 6902](https://tools.ietf.org/html/rfc6902).
Copy link
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
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
Copy link
Contributor

goern commented Dec 1, 2015

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
@@ -28,6 +28,10 @@
"description": "An optional boolean signifying the parameter should be obscured when displayed.",
"type": "boolean",
"default": false
}
},
Copy link
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
Copy link
Contributor

aweiteka commented Dec 2, 2015

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

@cdrage
Copy link
Member Author

cdrage commented Dec 2, 2015

@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
Copy link
Member Author

cdrage commented Dec 17, 2015

ping @aweiteka @goern updated again

@cdrage
Copy link
Member Author

cdrage commented Dec 21, 2015

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
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
Copy link
Member Author

cdrage commented Dec 22, 2015

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

@cdrage I submitted PR to update your branch.

Describe param as structured data
@cdrage
Copy link
Member Author

cdrage commented Jan 12, 2016

ping @aweiteka @bexelbie can we merge this?

@cdrage
Copy link
Member Author

cdrage commented Feb 11, 2016

timeout, pinging again @aweiteka @bexelbie

@goern
Copy link
Contributor

goern commented Feb 11, 2016

LGTM

@vpavlin ?

@bexelbie
Copy link

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
Copy link
Member Author

cdrage commented Mar 14, 2016

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

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

@dustymabe
Copy link
Contributor

ping @cdrage

@cdrage
Copy link
Member Author

cdrage commented Apr 7, 2016

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
Copy link
Member Author

cdrage commented Jun 6, 2016

@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