[RFC] tools: refactor all binary packages as library packages #2983

mfrw · 2022-05-11T09:04:01Z

Request for Comments

Closes: #2982

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

Summary

What does the PR accomplish, why was it needed?
In the current form, all the code in the binary packages is not re-useable to enable building on top of the great work we have.
This PR abstracts all the binary packages as library packages which can be imported in other packages as well to enable code-reuse.
The changes in this PR are 100% backward compatible and it does not break anything. To achieve this, the path of least resistance seemed
to expose a Config structure from the library package and pass all the relevant details that are passed as flags.

Change Log

tools: srpmpacker: refactor as a lib instead of a main package
tools: specreader: refactor as a lib instead of a main package
tools: pkgworker: refactor as a lib instead of a main package
tools: graphpkgfetcher: refactor as a lib instead of main package
tools: grapher: refactor as a lib instead of main package
tools: graphpreprocessor: refactor as a lib instead of main package
tools: mv imagegen -> pkg/imagegen
tools: imageconfigvalidator: refactor as a lib instead of main package
tools: imagepkgfetcher: refactor as a lib instead of main package
tools: imager: refactor as a lib instead of main package
tools: isomaker: refactor as a lib instead of main package
tools: roast: refactor as a lib instead of main package
tools: scheduler: refactor as a lib instead of main package
tools: liveinstaller: refactor as a lib instead of main package
tools: validatechroot: refactor as a lib instead of main package
tools: boilerplate: remove un-needed dir

Does this affect the toolchain?

YES

Associated issues

toolkit: refactor all binary packages as library packages to enable building on top of the toolkit #2982

Links to CVEs

NA

Test Methodology

Local Build

Signed-off-by: Muhammad Falak R Wani <[email protected]>

The internal directory in Go is a special directory. Any packages inside the 'internal' directory can't be imported across projects. This change exposes the 'logger' package as an exportable package. Signed-off-by: Muhammad Falak R Wani <[email protected]>

The internal directory in Go is a special directory. Any packages inside the 'internal' directory can't be imported across projects. This change exposes the 'pkggraph' as an exportable package. Signed-off-by: Muhammad Falak R Wani <[email protected]>

Signed-off-by: Muhammad Falak R Wani <[email protected]>

PawelWMS

I love the clean-up done here but would it be possible to split this change into smaller chunks? It's somewhat challenging to review all of this at once while maintaining full focus.

mfrw · 2022-05-19T01:25:11Z

I love the clean-up done here but would it be possible to split this change into smaller chunks? It's somewhat challenging to review all of this at once while maintaining full focus.

The individual commits in the PR are the smaller changes that achieve it.
Given this is such a big refactor, the reason for sending this huge PR is to chunk it as a logical change.
In my opinion, when we merge this, we should do a merge commit rather than a squash-and-merge to save the history.

Signed-off-by: Muhammad Falak R Wani <[email protected]>

PawelWMS · 2022-08-15T18:53:04Z

toolkit/tools/roast/roast.go

+		ImageTag:       *imageTag,
+	}
+}
+
 func main() {


Please let's keep main as the first function after the arguments and consts list - I wouldn't want to change that "convention".

PawelWMS · 2022-08-15T18:54:20Z

toolkit/tools/boilerplate/boilerplate.go

-	app = kingpin.New("boilerplate", "A sample golang tool for Mariner.")
-
-	logFile  = exe.LogFileFlag(app)
-	logLevel = exe.LogLevelFlag(app)


Why did we remove this one? I believe we were using it as a template for new tooling. If anything, we could update it now to follow your idea of organizing the tools.

PawelWMS · 2022-08-15T18:55:38Z

toolkit/tools/boilerplate/hello/hello.go

-// Copyright (c) Microsoft Corporation.
-// Licensed under the MIT License.
-
-package hello


Since we're removing this piece of code as well (and for good reasons, I believe), the commit message seems to vague. There's no indication this got removed.

PawelWMS · 2022-08-15T18:59:57Z

toolkit/tools/pkg/srpmpacker/srpmpacker.go

+	"github.com/microsoft/CBL-Mariner/toolkit/tools/internal/safechroot"
+)
+
+type fileSignaturesWrapper struct {


Just double-checking, because it's hard to tell from the diff (this shows as a brand new file): there are no changes in the code, comments, sections or functions ordering, etc., correct? It's moved 1:1 to a new directory and we're only adding the Config struct?

PawelWMS · 2022-08-15T19:00:49Z

toolkit/tools/srpmpacker/srpmpacker.go

@@ -122,6 +52,27 @@ var (
 	signatureHandling    = app.Flag("signature-handling", "Specifies how to handle signature mismatches for source files.").Default(signatureEnforceString).PlaceHolder(exe.PlaceHolderize(validSignatureLevels)).Enum(validSignatureLevels...)
 )

+func populateSrpmPackerConfig() *srpmpacker.Config {


Similar request here and all other commits - let's keep main as the first function.

PawelWMS · 2022-08-15T20:54:23Z

toolkit/tools/pkg/graph/grapher/grahper.go

@@ -0,0 +1,249 @@
+package grapher


Typo in file name grahper.go.

PawelWMS · 2022-08-15T21:03:10Z

toolkit/scripts/tools.mk

@@ -37,7 +36,7 @@ go_tool_targets = $(foreach target,$(go_tool_list),$(TOOL_BINS_DIR)/$(target))
 # Common files to monitor for all go targets
 go_module_files = $(TOOLS_DIR)/go.mod $(TOOLS_DIR)/go.sum
 go_internal_files = $(shell find $(TOOLS_DIR)/internal/ -type f -name '*.go')


I'm curious if we still need go_internal_files after all these changes.

PawelWMS · 2022-08-16T23:08:23Z

toolkit/tools/pkg/image/pkgfetcher/pkgfetcher.go

+	}
+
+	if err != nil {
+		logger.Log.Infof("Failed to clone RPM repo. Error: %s", err)


Why are errors logged on info level now? We can go down to error and make sure the caller does the right thing or keep the original code, like what we did in other commits.

PawelWMS · 2022-08-16T23:08:36Z

toolkit/tools/imagepkgfetcher/imagepkgfetcher.go

-	return
+	cfg := populateImagePkgFetcherConfig()
+	err := pkgfetcher.FetchPkgsAndCreateRepo(cfg)
+	logger.PanicOnError(err, "Failed to save cloned repo contents")


This looks incorrect now since pkgfetcher.FetchPkgsAndCreateRepo can fail for multiple different reasons.

PawelWMS · 2022-08-16T23:13:18Z

toolkit/tools/imager/imager.go

-	// the install directory
-	sshPubKeysTempDirectory = "/tmp/sshpubkeys"
+func populateImagerConfig() *imager.Config {
+	const defaultSystemConfig = 0


Looks like a duplicate from main and I think you're also not using the SystemConfig field below.

PawelWMS

I left a few comments. The ones I'm mostly interested in:

It seems we're not entirely consistent in how we deal with logger.log.Panic. In most cases we've left them unchanged, which seems fine, since we don't want to change the logic, just refactor, but there's at least one commit where I've seen them converted to logger.log.Info.
Related to the above comment: it's insanely difficult to go through all the changes and make sure the logic was not modified. Since the granularity of accepted changes is a whole PR, not separate commits, I'm finding it hard to maintain enough focus to feel safe about approving the whole thing. I'd strongly recommend splitting this into separate PRs to ease with te review process.

mfrw · 2022-08-19T04:02:22Z

I left a few comments. The ones I'm mostly interested in:

It seems we're not entirely consistent in how we deal with logger.log.Panic. In most cases we've left them unchanged, which seems fine, since we don't want to change the logic, just refactor, but there's at least one commit where I've seen them converted to logger.log.Info.

Related to the above comment: it's insanely difficult to go through all the changes and make sure the logic was not modified. Since the granularity of accepted changes is a whole PR, not separate commits, I'm finding it hard to maintain enough focus to feel safe about approving the whole thing. I'd strongly recommend splitting this into separate PRs to ease with te review process.

I appreciate the feedback comments. I think Yes, Let me pluck them one-by-one and try to create seperate PRs for an individual tool, using the same PR. I agree that this is a ginormous PR and reviewing it while keep full focus and not breaking anything is difficult :)

Again appreciate all the reviewers spending time on this PR.

PawelWMS · 2022-08-22T20:43:28Z

I left a few comments. The ones I'm mostly interested in:

It seems we're not entirely consistent in how we deal with logger.log.Panic. In most cases we've left them unchanged, which seems fine, since we don't want to change the logic, just refactor, but there's at least one commit where I've seen them converted to logger.log.Info.

Related to the above comment: it's insanely difficult to go through all the changes and make sure the logic was not modified. Since the granularity of accepted changes is a whole PR, not separate commits, I'm finding it hard to maintain enough focus to feel safe about approving the whole thing. I'd strongly recommend splitting this into separate PRs to ease with te review process.

I appreciate the feedback comments. I think Yes, Let me pluck them one-by-one and try to create seperate PRs for an individual tool, using the same PR. I agree that this is a ginormous PR and reviewing it while keep full focus and not breaking anything is difficult :)

Again appreciate all the reviewers spending time on this PR.

Thanks for your understanding, @mfrw! I understand this PR is an immense amount of work on your end. It looks super helpful and IMO close to done, so I'm going to keep an eye out for your updates and help with the review as much as possible.

mfrw added 2 commits May 9, 2022 11:58

tools: roast: refactor as a lib instead of main package

2ba8720

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: boilerplate: remove un-needed dir

eec64af

Signed-off-by: Muhammad Falak R Wani <[email protected]>

mfrw requested a review from a team as a code owner May 11, 2022 09:04

ghost added Tools main PR Destined for main labels May 11, 2022

mfrw added the DevEx Developer Experience label May 11, 2022

mfrw requested a review from jslobodzian May 11, 2022 09:52

nicogbg approved these changes May 11, 2022

View reviewed changes

mfrw requested review from dmcilvaney and PawelWMS May 11, 2022 15:34

christopherco self-requested a review May 11, 2022 22:16

mfrw added 2 commits May 12, 2022 10:16

mfrw force-pushed the mfrw/refactor-as-libraries branch from eb0445d to 0583fae Compare May 13, 2022 02:04

tools: mv 'internal/safechroot' -> 'pkg/safechroot'

202773d

Signed-off-by: Muhammad Falak R Wani <[email protected]>

oliviacrain approved these changes May 16, 2022

View reviewed changes

PawelWMS reviewed May 18, 2022

View reviewed changes

dmcilvaney approved these changes May 20, 2022

View reviewed changes

mfrw force-pushed the mfrw/refactor-as-libraries branch from 0583fae to b30d3ab Compare May 21, 2022 05:39

mfrw added 10 commits August 14, 2022 08:45

tools: srpmpacker: refactor as a lib instead of a main package

3e3c98c

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: specreader: refactor as a lib instead of a main package

78abafa

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: pkgworker: refactor as a lib instead of a main package

326364e

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: graphpkgfetcher: refactor as a lib instead of main package

d6ed34e

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: grapher: refactor as a lib instead of main package

40a13bf

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: graphpreprocessor: refactor as a lib instead of main package

f97832a

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: mv imagegen -> pkg/imagegen

6181abd

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: imageconfigvalidator: refactor as a lib instead of main package

8c6bacd

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: imagepkgfetcher: refactor as a lib instead of main package

0866397

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: imager: refactor as a lib instead of main package

4b73378

Signed-off-by: Muhammad Falak R Wani <[email protected]>

mfrw added 4 commits May 9, 2022 11:18

tools: isomaker: refactor as a lib instead of main package

124f312

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: scheduler: refactor as a lib instead of main package

2bf9744

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: liveinstaller: refactor as a lib instead of main package

01a0e49

Signed-off-by: Muhammad Falak R Wani <[email protected]>

tools: validatechroot: refactor as a lib instead of main package

58d6e44

Signed-off-by: Muhammad Falak R Wani <[email protected]>

mfrw force-pushed the mfrw/refactor-as-libraries branch 2 times, most recently from 4633033 to 202773d Compare August 15, 2022 04:01

PawelWMS reviewed Aug 15, 2022

View reviewed changes

toolkit/tools/pkg/graph/grapher/grahper.go

@@ -0,0 +1,249 @@

package grapher

Copy link

Contributor

PawelWMS Aug 15, 2022

Choose a reason for hiding this comment

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

Typo in file name grahper.go.

PawelWMS reviewed Aug 15, 2022

View reviewed changes

PawelWMS reviewed Aug 16, 2022

View reviewed changes

PawelWMS requested changes Aug 16, 2022

View reviewed changes

PawelWMS mentioned this pull request Sep 16, 2022

toolkit: added RPMs snapshots. #3734

Merged

12 tasks

mfrw mentioned this pull request Mar 30, 2023

Refactor toolkit/tools/internal -> toolkit/tools/pkg #5185

Open

12 tasks

mfrw mentioned this pull request Aug 16, 2023

Mariner Image Customizer boilerplate #5982

Merged

12 tasks

mfrw mentioned this pull request Feb 14, 2024

[RFC] tools: srpmpacker: refactor as a library #7887

Open

12 tasks

PawelWMS requested a review from a team as a code owner December 12, 2024 00:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] tools: refactor all binary packages as library packages #2983

[RFC] tools: refactor all binary packages as library packages #2983

mfrw commented May 11, 2022

PawelWMS left a comment

mfrw commented May 19, 2022

PawelWMS Aug 15, 2022

PawelWMS Aug 15, 2022

PawelWMS Aug 15, 2022

PawelWMS Aug 15, 2022

PawelWMS Aug 15, 2022

PawelWMS Aug 15, 2022

PawelWMS Aug 15, 2022 •

edited

Loading

PawelWMS Aug 16, 2022 •

edited

Loading

PawelWMS Aug 16, 2022

PawelWMS Aug 16, 2022

PawelWMS left a comment

mfrw commented Aug 19, 2022

PawelWMS commented Aug 22, 2022

[RFC] tools: refactor all binary packages as library packages #2983

Are you sure you want to change the base?

[RFC] tools: refactor all binary packages as library packages #2983

Conversation

mfrw commented May 11, 2022

Merge Checklist

Summary

Change Log

Does this affect the toolchain?

Associated issues

Links to CVEs

Test Methodology

PawelWMS left a comment

Choose a reason for hiding this comment

mfrw commented May 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PawelWMS Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

PawelWMS Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PawelWMS left a comment

Choose a reason for hiding this comment

mfrw commented Aug 19, 2022

PawelWMS commented Aug 22, 2022

PawelWMS Aug 15, 2022 •

edited

Loading

PawelWMS Aug 16, 2022 •

edited

Loading