Skip to content

docs/user/aws/images: Dia source for UPI arch diagram - #1525

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
wking:aws-upi-dia
Apr 18, 2019
Merged

docs/user/aws/images: Dia source for UPI arch diagram#1525
openshift-merge-robot merged 1 commit into
openshift:masterfrom
wking:aws-upi-dia

Conversation

@wking

@wking wking commented Apr 3, 2019

Copy link
Copy Markdown
Member

Dia is from @cuppett, replacing the PNG he'd submitted via 39a926a (#1408).

We aren't using the full file with all the layers yet, but I'm building it anyway because folks without Dia may still want to look at it ;).

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 3, 2019
Comment thread docs/user/aws/images/Makefile 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.

do you think will be worth it to add a note somewhere to say dia is a pre-req which can be installed using OS pkg tools - brew, apt, yum, dnf etc

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.

do you think will be worth it to add a note somewhere to say dia is a pre-req which can be installed using OS pkg tools - brew, apt, yum, dnf etc

Won't folks without it get a useful "command not found" error already? And I'd be surprised if all that many people were bumping the .dia source. So I'm currently leaning towards "punt". But I'd be fine landing docs if someone else wrote them up and they looked like they wouldn't go stale quickly ;).

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.

happy to add some notes (but will be in a week time once i'm back online)

Comment thread docs/user/aws/images/install_upi.svg Outdated
@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2019
Comment thread docs/user/aws/images/Makefile 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.

can we document how to build it / or a small script rather than add make for consistency?

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.

can we document how to build it / or a small script rather than add make for consistency?

We could, but it's going to take more space than the Makefile (e.g. we'd have to write our own code or text about checking for a pre-existing icon zip, unpacking the zip, etc. I don't expect a huge contributor base feeding diagram changes (although I guess you never know), so I'm ok with it how it stands, but am also happy to add docs (CONTRIBUTING.md? docs/user/aws/images/README.md? A Makefile header comment?) that say:

To rebuild the *.svg, run make in this directory.

@wking

wking commented Apr 5, 2019

Copy link
Copy Markdown
Member Author

I've pushed f5ee4a584 -> ed55423b9, adding install_upi*.svg and __MACOSX to the clean removals.

Dia is from Stephen Cuppett, replacing the PNG he'd submitted via
39a926a (Adding initial user doc/guide & materials for UPI AWS
installation, 2019-03-12, openshift#1408).

We aren't using the full file with all the layers yet, but I'm
building it anyway because folks without Dia may still want to look at
it ;).

SVGs generated with:

  $ dia --version
  Dia version 0.97.3, compiled 18:02:21 Feb 11 2017

relink-dia.py embeds the referenced icons in the SVG with def and use
[1,2] to avoid icon URIs like:

  file:///.../openshift/installer/docs/user/aws/images/./AWS-Architecture-Icons_SVG/Light-BG/_General%20AWS/AWS-General_AWS-Cloud_light-bg.svg

Ideally Dia would have a way to do this sort of thing automatically
with a command-line switch, but if it does, I can't find it.

[1]: https://developer.mozilla.org/en-US/docs/Web/SVG/Element/def
[2]: https://developer.mozilla.org/en-US/docs/Web/SVG/Element/use
@wking

wking commented Apr 5, 2019

Copy link
Copy Markdown
Member Author

I've waited long enough for the namespace to get reaped:

$ oc delete project ci-op-40fyf45z
Error from server (Forbidden): projects.project.openshift.io "ci-op-40fyf45z" is forbidden: User "wking" cannot delete projects.project.openshift.io in the namespace "ci-op-40fyf45z": no RBAC policy matched

/retest

@cuppett

cuppett commented Apr 15, 2019

Copy link
Copy Markdown
Member

@wking @abhinavdahiya I have an update to the DIA file for private masters ready to go, can this get LGTM and then I'll open a PR to update it?

@cuppett

cuppett commented Apr 15, 2019

Copy link
Copy Markdown
Member

/assign @abhinavdahiya

@abhinavdahiya

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2019
@openshift-ci-robot

Copy link
Copy Markdown
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

@openshift-merge-robot
openshift-merge-robot merged commit 9b2c5b9 into openshift:master Apr 18, 2019
@wking
wking deleted the aws-upi-dia branch April 23, 2019 02:47
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants