-
Notifications
You must be signed in to change notification settings - Fork 2k
Conversation
Signed-off-by: Phillip Whelan <[email protected]>
Please sign your commits following these rules: $ git clone -b "alpine" [email protected]:publicarray/machine.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842357248536
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f Amending updates the existing PR. You DO NOT need to open a new one. |
Signed-off-by: Sebastian Schmidt <[email protected]>
@frebib any comments/suggestions on this pr? |
In fact I actually need to try machine on an Alpine host so I'll give it another test on Monday and let you know. It's been a while since I last tested it |
libmachine/provision/alpine.go
Outdated
@@ -136,6 +136,11 @@ func (provisioner *AlpineProvisioner) Provision(swarmOptions swarm.Options, auth | |||
} | |||
} | |||
|
|||
log.Debug("Add Community repo") | |||
if _, err := provisioner.SSHCommand("if ! apk info docker >/dev/null; then ver=$(awk '{split($1,a,\".\"); print a[1]\".\"a[2]}' /etc/alpine-release); echo \"http://dl-cdn.alpinelinux.org/alpine/v$ver/community\" >> /etc/apk/repositories; apk update; fi"); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this could be better for detecting whether community is enabled. Strictly speaking docker could be installed but community not although I'm not entirely sure in this case that it matters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer if ! which docker >/dev/null && ! apk info docker >/dev/null; ...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll think about it. It's probably fine for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I've added the check in the last commit do you want me to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to summarise what (the updated) script does:
If the docker
command is not found and apk
can't find docker in any installed repositories via apk info docker
we continue to install the community repo. Next we retrieve the major and minor version from /etc/alpine-release
. The official mirror (CDN) for the appropriate version is appended to the /etc/apk/repositories
file. Finally apk update
is used to refresh the local repository database.
I think that this is pretty safe but if you have any ideas to improve this I'm open to improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just spent some time putting this together, tell me what you think:
grep '^[[:blank:]]*[^#].*community' /etc/apk/repositories || sed
-i '/^\s*[^#].*main/{p;s/main/community/;}' /etc/apk/repositories
This checks for the existence of an enabled community repo. If it's not present, it copies the main
repository and s/main/community/
.
From what I have tested this works in pretty much every case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think this is much better. Would you like to make the commit so you get credit for the script?
The only way i can think of where this would not work in the highly unlikely case where when 'main' is not installed. Maybe a user tries to run this script from the alpine.iso directly? from memory the iso is mounted as a cdrom and is the sole repository until setup-alpine is run. But again I don't think that will happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, hum. I didn't consider that case. Ideally this would be more complex to account for that too. Maybe I should add the extra check in if the main
repo doesn't exist either. I'll make a PR to your fork in a minute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to commit this later, but here is the revised statement:
if ! grep -q '^[[:blank:]]*[^#].*community$' /etc/apk/repositories; then grep -q '^[[:blank:]]*[^#].*main$' /etc/apk/repositories || echo "http://dl-cdn
.alpinelinux.org/alpine/v$(cut -d. -f1-2 /etc/alpine-release)/main" >> /etc/apk/repositories; sed -i '/^\s*[^#].*main/{p;s/main/community/;}' /etc/apk/repositories; fi; cat /etc/apk/repositories
libmachine/provision/alpine.go
Outdated
case pkgaction.Install, pkgaction.Upgrade: | ||
packageAction = "add" | ||
case pkgaction.Remove: | ||
packageAction = "remove" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ apk remove
ERROR: 'remove' is not an apk command. See 'apk --help'.
This should be:
packageAction = "del"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks
Signed-off-by: Sebastian Schmidt <[email protected]>
Ah 'Signed-off-by:' is missing in some commits. I'll give you push rights to my fork. I think that is the easiest way to fix this. |
You want me to rebase and sign-off my commits? Ok I think we're good now |
Adds a default dl-cdn community repo if main is absent, based on /etc/alpine-release Signed-off-by: Joe Groocock <[email protected]>
Thanks! Yea, I'm also new with this Signed-off-by stuff. |
Hi! Unfortunately, Machine is now in maintenance mode, meaning we're not merging new features or new provisioners anymore. Thank you for your understanding. |
Permanently? |
Barring some dramatic changes at Docker, yes! |
Well it was nice to have some kind of announcement. I guess Docker has forgotten what 'community' means. |
I'm sorry you feel slighted by this - it has been the intended direction for over a year now, even though I'll admit it has been poorly communicated. I opened #4537 as a more visible announcement, as well as a channel for further discussion. |
I think with the backtics the extra escaping is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to sign-off this commit
@frebib Thanks but since this won't be merged I think it's pointless to sign-off any further commits. I'll keep the fork around and close this PR. Thanks so much on helping though. Much appreciated. |
for those of you needing machine, there is some activity in the https://github.com/machine-drivers organisation, and it may make sense for you to work on, and release from https://github.com/machine-drivers/machine... |
I cherry-picked the commit at #4106 and added a shell script to add the community repo if it's not already installed. I tried to make it work even if the /etc/apk/repositories file was modified beforehand, but as a result the mirror is hard coded to the official alpine cdn