Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Sep 12, 2018

Now that these structures are defined under pkg/type/, dropping this file avoids either the content going stale (if we don't keep it up to date) or tedious updates (if we do keep it up to date). Consumers who want details on the structure can use the godocs.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 12, 2018
@abhinavdahiya
Copy link
Contributor

nit: As a lot of operator teams will be depending on this struct, we will very often be sharing a link to InstallConfig so maybe add this link https://godoc.org/github.com/openshift/installer/pkg/types#InstallConfig as comment. It will be easier to directly copy from there when needed.

Otherwise i'm okay with dropping if keeping it in sync is a problem.
/cc @crawford

@crawford
Copy link
Contributor

I'd rather generate docs from code than try to keep things up-to-date by hand.

/approve

@wking
Copy link
Member Author

wking commented Sep 12, 2018

... so maybe add this link https://godoc.org/github.com/openshift/installer/pkg/types#InstallConfig as comment.

You mean beyond the the godocs link in my initial PR comment? Or somewhere in the Git repo itself (somewhere (new?) under Documentation/?)? Maybe in the README and libvirt howto?

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Sep 12, 2018

You mean beyond the the godocs link in my initial PR comment? Or somewhere in the Git repo itself

README sounds good?

@wking wking force-pushed the drop-install-config-design-doc branch from 5d661f0 to 9ac6e0d Compare September 13, 2018 00:03
@wking
Copy link
Member Author

wking commented Sep 13, 2018

README sounds good?

Done with 5d661f0 -> 9ac6e0d.

@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2018
Now that these structures are defined under pkg/type/, dropping this
file avoids either the content going stale (if we don't keep it up to
date) or tedious updates (if we do keep it up to date).  Consumers who
want details on the structure can use [1].

[1]: https://godoc.org/github.com/openshift/installer/pkg/types#InstallConfig
@wking wking force-pushed the drop-install-config-design-doc branch from 9ac6e0d to d006d8c Compare September 13, 2018 04:18
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2018
@wking
Copy link
Member Author

wking commented Sep 13, 2018

Rebased around #237 with 9ac6e0d -> d006d8c.

Copy link
Contributor

@abhinavdahiya abhinavdahiya left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,crawford,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 3c76acd into openshift:master Sep 13, 2018
@wking wking deleted the drop-install-config-design-doc branch September 13, 2018 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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.

5 participants