Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Jul 9, 2018

The O_RDWR is originally from 1b4bb62 (coreos/tectonic-installer#2806), but we do not require the ability to read from the target file.

Also add a unit test to exercise this code.

@coreosbot
Copy link

Can one of the admins verify this patch?

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2018
@wking
Copy link
Member Author

wking commented Jul 9, 2018

Hold off on this one, there's another copyFile with the same issue in pkg/config-generator/utils.go. I'll reroll this shortly to pull it out into a separate pkg.

@wking wking force-pushed the copy-target-write-only branch from 037af30 to a3976a2 Compare July 9, 2018 08:38
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2018
@wking
Copy link
Member Author

wking commented Jul 9, 2018

Hold off on this one, there's another copyFile...

Ok, I've pushed 037af304 -> 59829963, creating a new installer/pkg/copy to stay DRY.

@wking wking force-pushed the copy-target-write-only branch from a3976a2 to 5982996 Compare July 9, 2018 08:41
@wking wking changed the title installer/pkg/workflow/utils: Change O_RDWR to O_WRONLY in copyFile installer/pkg/copy: Factor out Copy into its own package Jul 9, 2018
@yifan-gu
Copy link
Contributor

lgtm to me, /cc @trawler or @enxebre for another pair of eyes.

@yifan-gu
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 11, 2018
@yifan-gu
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2018
@enxebre
Copy link
Member

enxebre commented Jul 12, 2018

lgtm

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2018
This code is descended from terraformPrepareStep, which landed in
1b4bb62 (Cli tool, 2018-02-05, coreos/tectonic-installer#2806).  The
workspace version was factored out into copyFile in 2b82fbe
(installer: Integrate multistep cli with configuration, 2018-02-16,
coreos/tectonic-installer#2960).  The config-generator version was
copy/pasted (or independently developed?) in 1d54899
(installer/pkg/config-generator/tls: generate root CA's with go,
2018-06-28, coreos/tectonic-installer#3316).  This commit DRYs that up
by pulling the duplicate code out into a shared package.

I've also changed O_RDWR to O_WRONLY.  The O_RDWR is originally from
1b4bb62, but we do not require the ability to read from the target
file.

Also add a unit test to exercise this code.
@wking wking force-pushed the copy-target-write-only branch from 5982996 to 9abf7a3 Compare July 16, 2018 03:50
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2018
@wking
Copy link
Member Author

wking commented Jul 16, 2018

@openshift-bot openshift-bot added the needs-rebase label...

Not sure why this just came in now, it looks like the issues were context conflicts with #42, which landed Friday. Anyway, rebased with onto master with 5982996 -> 9abf7a3.

@yifan-gu
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2018
@openshift-merge-robot openshift-merge-robot merged commit 4d5ff8a into openshift:master Jul 18, 2018
@wking wking deleted the copy-target-write-only branch July 19, 2018 23:54
mkumatag added a commit to mkumatag/installer that referenced this pull request Aug 6, 2021
clnperez pushed a commit to clnperez/installer that referenced this pull request Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

7 participants