Skip to content

Extend the Flatcar Container Linux installer script to include a -D option for local image download#37

Merged
pothos merged 1 commit intoflatcar:flatcar-masterfrom
jasonbraganza:mjb-adapt-init-2
Mar 22, 2021
Merged

Extend the Flatcar Container Linux installer script to include a -D option for local image download#37
pothos merged 1 commit intoflatcar:flatcar-masterfrom
jasonbraganza:mjb-adapt-init-2

Conversation

@jasonbraganza
Copy link
Copy Markdown
Contributor

@jasonbraganza jasonbraganza commented Mar 10, 2021 •
edited
Loading

Title: Added flag to flatcar-install to only download the image file, as required by issue #248

The install_from_url script was refactored.
We now have 3 functions

  • prep_url handles setting of variables and gpg keys for the download
  • install_from_url does the original task of downloading and writing to a disk
  • download_from_url downloads the bz2 image to the current folder and saves it.

Closes flatcar/Flatcar#248

How to use

run flatcar-install -D and the script should download the image to the current folder

Testing done

Have tried downloading the image across two computers.
Has successfully executed on one.
Still downloading on the other (results expected)

pothos
pothos reviewed Mar 10, 2021
View reviewed changes
pothos
pothos reviewed Mar 10, 2021
View reviewed changes
pothos
pothos reviewed Mar 10, 2021
View reviewed changes
pothos
pothos reviewed Mar 10, 2021
View reviewed changes
margamanterola
margamanterola reviewed Mar 10, 2021
View reviewed changes
@jasonbraganza
Copy link
Copy Markdown
Contributor Author

jasonbraganza commented Mar 10, 2021

@marga-kinvolk, @pothos thank you both. will work on all you’ve said.

pothos
pothos reviewed Mar 10, 2021
View reviewed changes
@jasonbraganza
Copy link
Copy Markdown
Contributor Author

jasonbraganza commented Mar 11, 2021 •
edited
Loading

working on all of this. almost done. will update once i am done testing.

@jasonbraganza
Copy link
Copy Markdown
Contributor Author

jasonbraganza commented Mar 11, 2021

d2

@marga-kinvolk @pothos - done. would be grateful for further advice or action.

pothos
pothos reviewed Mar 11, 2021
View reviewed changes
pothos
pothos reviewed Mar 11, 2021
View reviewed changes
margamanterola
margamanterola reviewed Mar 11, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@margamanterola margamanterola left a comment

Choose a reason for hiding this comment

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

Thanks, this looks much better. Just one comment about the command documentation and two nits (this is small comments that are not very important).

Once you've solved this, would you please squash your commits into just one and make sure that the commit description has the right information? Thanks.

@jasonbraganza
flatcar-install: Add -D flag to download in directory
1497d94 
The -D flag will download Flatcar Container Linux image to the
current directory, without installing it

Signed-off-by: Mario Jason Braganza <jason@janusworx.com>
@jasonbraganza
Copy link
Copy Markdown
Contributor Author

jasonbraganza commented Mar 12, 2021

Te squash and commit are done.
(lots of thanks to @sayanchowdhury, for helping me out with git hiccoughs)

pothos
pothos approved these changes Mar 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@pothos pothos left a comment

Choose a reason for hiding this comment

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

Gave it another test run and it works good, thanks!
A minor thing is that umask 077 makes the downloaded file restricted to the downloading user which could be reverted afterwards (but not skipped because it's maybe relevant for the verification keys in /tmp)

@jasonbraganza
Copy link
Copy Markdown
Contributor Author

jasonbraganza commented Mar 22, 2021

what would you suggest the mask be, @pothos? i could try working on that aspect too

@pothos
Copy link
Copy Markdown
Member

pothos commented Mar 22, 2021

I think it's out of scope, but I would say before running umask 077 the previous umask is saved (PREV_UMASK=$(umask)) and then in the download function the umask is restored before wget starts (umask ${PREV_UMASK}).

@jasonbraganza
Copy link
Copy Markdown
Contributor Author

jasonbraganza commented Mar 22, 2021

@pothos just so i understand correctly, you want the downloaded file to have 755 permissions so the download ought to run with a 022 mask and then set back to 077?

as for scope, well, i want to learn and do, so no worries there :)

@pothos
Copy link
Copy Markdown
Member

pothos commented Mar 22, 2021

I meant we can first merge the PR as is if you want :) Yes, the umask is done to limit the temporary gpg folder I think and it's good to keep it, but I would just reset the umask to the previous value before the download starts. This way the user can still control the final permissions by tweaking umask before starting the download script.

@jasonbraganza
Copy link
Copy Markdown
Contributor Author

jasonbraganza commented Mar 22, 2021

@pothos aah that sounds a teensy bit involved, so yes, i’ll wait for the pr to be merged and do this as a separate pr, if it’s ok with you folks :)

@pothos
Copy link
Copy Markdown
Member

pothos commented Mar 22, 2021

Yes, thanks again

@pothos pothos merged commit 6b7d062 into flatcar:flatcar-master Mar 22, 2021
@pothos
Copy link
Copy Markdown
Member

pothos commented Mar 22, 2021

I think it's out of scope, but I would say before running umask 077 the previous umask is saved (PREV_UMASK=$(umask)) and then in the download function the umask is restored before wget starts (umask ${PREV_UMASK}).

Here again the suggestion in a better format:

diff --git a/bin/flatcar-install b/bin/flatcar-install
index 25a8db4..088f841 100755
--- a/bin/flatcar-install
+++ b/bin/flatcar-install
@@ -41,6 +41,7 @@ find_smallest_unused_disk() {
     echo "$smallestdisk"
 }
 
+PREV_UMASK=$(umask)
 # Everything we do should be user-access only!
 umask 077
 
@@ -552,6 +553,7 @@ function prep_url(){
 function download_from_url(){
     prep_url
     echo "Downloading, writing and verifying ${IMAGE_NAME}..."
+    umask ${PREV_UMASK}
     if ! wget ${WGET_ARGS} --no-verbose -O "${PWD}/${IMAGE_NAME}" "${IMAGE_URL}"; then
         echo "Could not download ${IMAGE_NAME}" >&2
         exit 1

@jasonbraganza
Copy link
Copy Markdown
Contributor Author

jasonbraganza commented Mar 23, 2021

Thank you @pothos for showing me that what you were suggesting is in fact, simple :)

Thank you so much @marga-kinvolk and @pothos for guiding and herding me along. This is my first major contribution to an open source project. Feels really fun :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pothos pothos pothos approved these changes

+1 more reviewer

@margamanterola margamanterola margamanterola left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add flag to flatcar-install to only download the image file

3 participants

@jasonbraganza @pothos @margamanterola