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

Design doc csv generation #516

Merged
merged 6 commits into from
Nov 29, 2018
Merged

Conversation

estroz
Copy link
Member

@estroz estroz commented Sep 21, 2018

This PR contains a first stab at a design doc describing SDK generation of ClusterServiceVersion's (CSV's) for operators. Mechanisms and conventions described in this draft are suggestions, not necessarily features.

Check out #673 for a WIP implementation.

@estroz estroz added design needs discussion do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 21, 2018
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 21, 2018
Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Few questions

doc/design/milestone-0.0.7/csv-generation.md Outdated Show resolved Hide resolved
doc/design/milestone-0.0.7/example.describe.yaml Outdated Show resolved Hide resolved
doc/design/milestone-0.0.7/csv-generation.md Outdated Show resolved Hide resolved
@robszumski
Copy link
Contributor

This looks great!

@estroz estroz changed the title [WIP] Design doc csv generation Design doc csv generation Oct 2, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2018
@estroz estroz force-pushed the design-doc-csv branch 4 times, most recently from e0d18d6 to 6f3918f Compare October 22, 2018 21:43
@ecordell
Copy link
Member

This looks great, can't wait to try it out 😄

Should we talk about additional management features here, or should they go in another proposal? One concern of mine is helping sdk users understand how to build sets of updates, not just a single CSV. This would be dealing with channels, packages, replaces, etc. But these are all "a layer up" from what this proposal addresses.

@estroz
Copy link
Member Author

estroz commented Oct 30, 2018

@ecordell yeah that stuff should go in another proposal. This one is mostly for how CSV's will be generated from input data.

@shawn-hurley
Copy link
Member

I am having a pretty difficult time following the proposal, and because of this, I am worried that I might be missing gaps in regards to the ansible operator. Sorry for not getting it.

Two things I notice even without following:

  1. you mention aliasing the types and then adding a Apply method on them but in the example, you seem to be embedding the type and not aliasing it.

  2. I don't understand where the user-supplied values are coming from and from what I think I read you will error if these are not set.

  3. We are doing this on a build, but it seems that we will only ever have a single csv in the deploy folder? did I miss understand?

  4. Are we sure that the yaml files in deploy (which seem to be mostly for "here get your thing running") are the correct source of truth that we should rely on?

What I might be proposing here is a different directory with different rules that might be bootstrapped from deploy but allows us to keep the deploy scripts "here run x and now your working" while another directory can start to remove stuff to take advantage of OLM offerings. Maybe this is starting to think about the "next level" stuff that Evan mentions.

It might be nice to help me understand if you add an expected user workflow section? with the different use cases, we are solving and what you expect the commands to be and what the output of each would be?

@estroz
Copy link
Member Author

estroz commented Oct 31, 2018

Points from our discussion of @shawn-hurley's comment out-of-band:

  1. Changing alias -> embed in docs
  2. User-supplied values are those not discernible from codified data, ex. English descriptions. Instead of raising errors when certain values are not present, the SDK will raise warnings. I will add notes on how this will work.
  3. A larger discussion will be had about how we want to store CSV files, since there are two streams of thought:
    1. Always have one csv.yaml (like an RPM spec)
    2. Version the file itself, ex. csv.x.y.z.yaml
  4. We may want to include a configuration file or extra flags so users can specify what files/directories they want parsed to create a CSV. Configuration is particularly important for users that have different testing and production data.

I'm going to add more notes on CSV generation/usage workflow, as well as a flow diagram.

@estroz estroz requested review from mhrivnak and shawn-hurley and removed request for rithujohn191 and fanminshi October 31, 2018 20:12
@estroz estroz force-pushed the design-doc-csv branch 3 times, most recently from e60bc71 to 9e88920 Compare November 2, 2018 00:41
@hasbro17
Copy link
Contributor

Overall SGTM. Just need to clarify the nit on how the generator knows the CSV version while generating it.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Few nits and questions, but overall sounds sane to me :)

doc/design/milestone-0.2.0/csv-generation.md Outdated Show resolved Hide resolved
doc/design/milestone-0.2.0/csv-generation.md Outdated Show resolved Hide resolved
doc/design/milestone-0.2.0/csv-generation.md Outdated Show resolved Hide resolved
@estroz estroz force-pushed the design-doc-csv branch 2 times, most recently from b8e4d33 to 6c734b3 Compare November 19, 2018 23:31
Copy link
Member

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

LGTM

I would like to see the answer to which command this will be apart of.

I will say that this does appear to be adding much more complexity to the build command, which will get more complex once we get the manage dockerfiles stuff completed. I think we may want to keep that in mind.

Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM

I think this is good enough for the proposal.
Once we start iterating on the actual implementation we'll see how we can revisit the TODO's for managing multiple CSV versions.

@estroz estroz merged commit 07ed6d2 into operator-framework:master Nov 29, 2018
@estroz estroz deleted the design-doc-csv branch November 29, 2018 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design needs discussion size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants