Few fixes from after #105 #130
Conversation
mmlb
force-pushed
the
fixups-from-previous-refactors
branch
from
40243b5 to
06f5ff7
Compare
mmlb
force-pushed
the
fixups-from-previous-refactors
branch
from
06f5ff7 to
a3b5389
Compare
rules.mk
Outdated
| out/$T/$(1)/hook_$(DOCKER_2_HW_$(2)).tar.gz: out/$T/$(1)/initramfs-$(DOCKER_2_HW_$(2)) out/$T/$(1)/vmlinuz-$(DOCKER_2_HW_$(2)) | ||
| out/$T/$(1)/initramfs-$(DOCKER_2_HW_$(2)): out/$T/$(1)/$(2)/initrd.img | ||
| out/$T/$(1)/vmlinuz-$(DOCKER_2_HW_$(2)): out/$T/$(1)/$(2)/kernel | ||
| dist-files += out/$T/$(1)/initramfs-$(DOCKER_2_HW_$(2)) out/$T/$(1)/vmlinuz-$(DOCKER_2_HW_$(2)) |
There was a problem hiding this comment.
I encourage you to use less make over more. this addition is difficult to understand.
There was a problem hiding this comment.
Not a fan of having make do unnecessary work but I hear ya, this was new to me too. So its gone now. I was able to give the macro parameters better names (something I tried before but didn't work out until I changed some things for other reasons). Can you take a look at the last commit and see if that makes things a little clearer?
mmlb
force-pushed
the
fixups-from-previous-refactors
branch
2 times, most recently
from
4cfaa2d to
8ca64bb
Compare
|
Hey @ScottGarman @jacobweinstock can y'all take another look please? |
|
Looking and FYI this will need @thebsdbox or @tstromberg in order to get merged. |
| @@ -94,8 +94,8 @@ services: | |||
| files: | |||
| - path: etc/profile.d/local.sh | |||
| contents: | | |||
| alias docker='ctr -n services.linuxkit tasks exec --tty --exec-id cmd docker docker' | |||
| alias docker-shell='ctr -n services.linuxkit tasks exec --tty --exec-id shell docker sh' | |||
| alias docker='ctr -n services.linuxkit tasks exec --tty --exec-id cmd hook-docker docker' | |||
There was a problem hiding this comment.
is this extra space intentional?
There was a problem hiding this comment.
yep just an alignment along the = so that its easy to spot the differences.
|
@jacobweinstock you're still pending as "requested changes" afaik all have been addressed. Is there anything else? |
Haven't seen much activity from either in this repo in a while, an exception may be in order. I've also opened up #131 to add myself as a maintainer. |
sure, you already have approver permissions but don't know how moving to maintainer helps get PR's from you merged though. I think another approver would be a more idea option over an exception/override from a maintainer. |
Yeah. I figure we always want someone "above the ladder" to approve. But then what do we do about maintainers? IDK if maintainers need approval from other maintainers or if we go "one rung down the ladder" for those, or both. |
|
I haven't seen much interaction from @thebsdbox on this repo, ditto @tstromberg who coincidentally has a single commit for auxillary code (lint-install setup). idk if makes sense to use that commit as the basis for approver status. This repo all-in-all is in need of more active people in the roles, or loosening of gates. |
needing more active folks is definitely what we should be focusing on. loosening gates doesn't really make sense. As you have noted Thomas only has a single commit and yet he is still an approver. Also, the current governance handles approving PRs the same for all people who open a PR. It doesnt matter what the role of that person is. The process is the same. |
Hmm I guess I was confused about the approver role, for some reason I thought another maintainer would be wanted to approve of a mainter's code. That not being the case makes a ton of sense. At this point though, this repo has just one active member that can approve PRs, me. Let me put out a call for approvers in slack. |
mmlb
force-pushed
the
fixups-from-previous-refactors
branch
from
8ca64bb to
02d623c
Compare
mmlb
force-pushed
the
fixups-from-previous-refactors
branch
from
02d623c to
388d1aa
Compare
Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>
I didn't notice that when I previously re-worked the make setup that I changed the names away from what boots expects. Boots expect gnu style arch names where docker use go names (x86_64 vs amd64 and aarch64 vs arm64). I ended up coming across a way to sort of do a staticly defined look up table in make using something called "Constructed Macro Names", see http://make.mad-scientist.net/constructed-macro-names/ for details. So now we can use make to do the cp instead of in shell. I also reworked the deploy target so it pushes multiple files instead of one tarball as thats how boots and sandbox use them, this way we avoid needing to post-process the deployed artifacts. But this is disabled at the moment as pushing to the s3 bucket is failing due to issue with creds. Once we get creds fixed up this can be re-enabled. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>
This was nice to let make be aware of all the dependencies but it was making use of some uncommon makeisms. Seeing as `dist` is a leaf target anyway its not super critical to avoid running unnecessarily. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>
This is currently broken anyway. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>
I did not notice that the docker being referenced here before was for the docker service that was renamed hook-docker. This went by unnoticed since I was booting old versions of hook that did not have the local.sh file embedded. I finally figured out I had broken the aliases when I renamed the server after actually booting the hook images I built. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>
This wasn't possible in initial iterations but after some refactors I thought it might be doable again. Indeed it is and this makes it easier to see whats going on in the macros. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>
mmlb
force-pushed
the
fixups-from-previous-refactors
branch
from
388d1aa to
2edb728
Compare
## Description - Makes CI green again by avoiding attempting to push the linuxkit images via `make deploy` which was deleted. - Removes the dbg only binding of /etc/daemon.json which breaks hook-docker since it ends up being mounted read-only. ## Why is this needed Fixes CI and dbg runs. ## How Has This Been Tested? CI fix should just work. I've been running with the daemon.json drop in a different branch and thought it was in branch for #130. ## How are existing users impacted? What migration steps/scripts do we need? CI is ✔️ not ❌ which makes us all feel warm and fuzzy. dbg builds are actually useful and work.
Hello. I would like to request moving from reviewer to approver role. I am looking for a sponsor from either @mmlb or @thebsdbox. Please and thank you. Requirements: - [X] I have reviewed the [community membership guidelines](https://github.com/tinkerbell/proposals/blob/main/proposals/0024/GOVERNANCE.md) - [X] I have [enabled 2FA on my GitHub account](https://github.com/settings/security) - [X] I have subscribed to the [tinkerbell-contributors e-mail list](https://lists.cncf.io/g/cncf-tinkerbell-dev) - [X] I am actively contributing to 1 or more Tinkerbell subprojects - [X] Reviewer for at least 1 month - [X] Shallow understanding of the technical goals and direction of the repository - [X] Shallow understanding of the technical domain of the repository - [X] Reviewed or merged at least 3 substantial PRs to the codebase such as significant re-design or whole new features PR Reviews #98 #89 #130 PR merges #118 #117 #116 #115 #59 Sponsor Request from @mmlb @thebsdbox
Hello. I would like to request moving from reviewer to approver role. I am looking for a sponsor from either @mmlb or @thebsdbox. Please and thank you. Requirements: - [X] I have reviewed the [community membership guidelines](https://github.com/tinkerbell/proposals/blob/main/proposals/0024/GOVERNANCE.md) - [X] I have [enabled 2FA on my GitHub account](https://github.com/settings/security) - [X] I have subscribed to the [tinkerbell-contributors e-mail list](https://lists.cncf.io/g/cncf-tinkerbell-dev) - [X] I am actively contributing to 1 or more Tinkerbell subprojects - [X] Reviewer for at least 1 month - [X] Shallow understanding of the technical goals and direction of the repository - [X] Shallow understanding of the technical domain of the repository - [X] Reviewed or merged at least 3 substantial PRs to the codebase such as significant re-design or whole new features PR Reviews #98 #89 #130 PR merges #118 #117 #116 #115 #59 Sponsor Request from @mmlb @thebsdbox
Description
Some fixes from after #105 was merged.
sedso debug builds actually have the dbg stuff.make distmatch the file names expected by boots and sandbox.deploytarget for the new names, but have disabled it since publish is broken due to bad creds (we are also waiting on a cncf managed s3 bucket).Why is this needed
Fixes things broken by refactor.
How Has This Been Tested?
Manual tests.