Skip to content

Conversation

@runcom
Copy link
Member

@runcom runcom commented Feb 13, 2019

Please go over it by testing the flow as well to make sure it's correct (I just did and it works for me (tm)).
Please also correct any of my mistakes in english lol

Signed-off-by: Antonio Murdaca [email protected]

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 13, 2019
@runcom runcom force-pushed the docs-custom-release-payload branch from 5e1dfa7 to 35f01bb Compare February 13, 2019 21:11
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Looks great to me! Looking forward to trying this out.

docs/HACKING.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

There's also make image-controller make image-daemon etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

omg, how have I missed those 😥

@runcom runcom force-pushed the docs-custom-release-payload branch 3 times, most recently from a39246e to ad7cd76 Compare February 13, 2019 21:21
Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment

Choose a reason for hiding this comment

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

Some minor comments. This is great!

docs/HACKING.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

"tesing" -> "testing"

docs/HACKING.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

i think delete "for any of the component"

docs/HACKING.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

"that would" -> "that"

docs/HACKING.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

"into" -> "in"

docs/HACKING.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

"any of the" -> "any"

@kikisdeliveryservice
Copy link
Contributor

Generally, I'm wondering at what point we start breaking some of these sections out into their own pages and just link to them in HACKING.MD bc it's getting long.

@runcom
Copy link
Member Author

runcom commented Feb 13, 2019

Generally, I'm wondering at what point we start breaking some of these sections out into their own pages and just link to them in HACKING.MD bc it's getting long.

I can do that as part of this PR if you want to, any idea on how to break it down to different pages?

@kikisdeliveryservice
Copy link
Contributor

@runcom I'm ok with not doing it here. I think once this is in, we can take stock of what we have and decide how we'd like to organize it going forward. 😄

docs/HACKING.md Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

there's a known limitation now in code for this though #421, which can be worked around with #421 (basically all operands have to be at the very same version or the installation just fails).
That can be worked around by bundling all the components though (see #421 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

Probably worth just mentioning this limitation and link to the issue directly in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed

@runcom
Copy link
Member Author

runcom commented Feb 13, 2019

/hold

Holding due to https://github.com/openshift/machine-config-operator/pull/425/files#r256622962 but please keep testing this 🎉

@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 Feb 13, 2019
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Some minor tweaks, otherwise LGTM!

docs/HACKING.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, seems like this sentence should be before the previous one?

docs/HACKING.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

user

docs/HACKING.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

simply

@runcom runcom force-pushed the docs-custom-release-payload branch from ad7cd76 to 5cc653c Compare February 14, 2019 17:38
@runcom
Copy link
Member Author

runcom commented Feb 14, 2019

/hold cancel

because I've addressed #425 (comment)

@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 Feb 14, 2019
@kikisdeliveryservice
Copy link
Contributor

I'm a little confused here what am I supposed to do for this particular part
--to-image quay.io/user/origin-release:v4.0 \

I get building the mco components and adding but am I supposed to somehow build origin-release:v4.0??

@runcom
Copy link
Member Author

runcom commented Feb 14, 2019

I get building the mco components and adding but am I supposed to somehow build origin-release:v4.0??

that image is the output of oc adm release new itself, it gets produced as part of that command and gets automagically pushed as well so you can use it with the installer

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019
@runcom runcom force-pushed the docs-custom-release-payload branch from 5cc653c to 655ec6e Compare February 14, 2019 20:50
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019
@runcom
Copy link
Member Author

runcom commented Feb 14, 2019

repushed cause I've missed some backslashes (thanks @kikisdeliveryservice )

@kikisdeliveryservice
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, kikisdeliveryservice, runcom

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 [cgwalters,kikisdeliveryservice,runcom]

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

@rphillips
Copy link
Contributor

Thanks for writing this up. Looks awesome!

@openshift-merge-robot openshift-merge-robot merged commit e4d3b48 into openshift:master Feb 14, 2019
@runcom runcom deleted the docs-custom-release-payload branch February 14, 2019 21:18
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants