Skip to content

Conversation

@abhinavdahiya
Copy link
Contributor

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 9, 2018
@smarterclayton
Copy link
Contributor

/retest

Copy link
Member

Choose a reason for hiding this comment

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

To stay DRY, we should put these lists under variables in targets.go. Maybe:

type target struct {
  name string
  command *cobra.Command
  assets []asset.Asset
}

var targets := map[string]target{
  "install-config": {
    name: "Install Config",
    command: &cobra.Command{
      Use:   "install-config",
      Short: "Generates the Install Config asset",
    },
    assets: {&installconfig.InstallConfig{}},
  },
  ...
}

Then in main():

for key, target := range targets { // maybe after sorting?
  target.command.RunE = runTargetCmd(target.assets...)
  subCmds = append(subCmds, target.command)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

@wking wking Oct 9, 2018

Choose a reason for hiding this comment

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

Do we need PkgPath? elem.Name seems fairly unique on its own. Or maybe make the displayed name elem.Name, but make the node href point to the associated godocs? For SVG output (and anything else that supports hrefs) that would give you a compact display with fully-qualified names available as needed to clickers and hoverrers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It adds context if anybody wants to find the asset.

@abhinavdahiya abhinavdahiya force-pushed the cli_graph branch 2 times, most recently from ff38a6c to 772c014 Compare October 9, 2018 16:37
Copy link
Member

Choose a reason for hiding this comment

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

This still duplicates the assets peoperty two lines below. Not a big deal, because they're so close together, but does building RunE on the fly not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, forgot to remove that.

Copy link
Member

Choose a reason for hiding this comment

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

The trim leaves this without the required leading quote. And I'd still prefer context via URL. Something like:

InstallConfig [URL="https://godoc.org/github.com/openshift/installer/pkg/asset/installconfig#InstallConfig"]

For private classes, maybe just link to the package?

To make it easier to tune this, maybe add a new commit updating our dependency SVG and dropping our current hand-coded dot source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dropped the name to just name of elem for now. we can iterate over the url stuff later.
Also adding the commit for replacing existing graph in docs.

@abhinavdahiya abhinavdahiya force-pushed the cli_graph branch 2 times, most recently from bb81795 to 9d5d1a9 Compare October 9, 2018 17:46
@abhinavdahiya
Copy link
Contributor Author

@wking updated to include doc.

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Oct 9, 2018

Choose a reason for hiding this comment

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

github.com/awalterschulze/gographviz cannot print strict digraph.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can drop resource_dep.dot from version control and just use:

bin/openshift-install graph | dot -Tsvg >docs/design/resource_dep.svg

Also, with sh highlighting, you don't want the $ prompts (more on sh vs. console in #27.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking done.

```sh
bin/openshift-install graph | dot -Tsvg >docs/design/resource_dep.svg
```
Copy link
Member

@wking wking 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 Oct 9, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, 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,wking]

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

@abhinavdahiya
Copy link
Contributor Author

/test e2e-aws

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@wking
Copy link
Member

wking commented Oct 9, 2018

e2e:

Ran for 41s
error: could not run steps: could not wait for template instance to be ready: could not determine if template instance was ready: failed to create objects: object is being deleted: pods "e2e-aws" already exists

@openshift-merge-robot openshift-merge-robot merged commit ee9168f into openshift:master Oct 9, 2018
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants