Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor/ci tasks improvement new #5303

Merged
merged 6 commits into from
Oct 19, 2023
Merged

refactor/ci tasks improvement new #5303

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
532abf7
refactor: fix generate-kustimize-builtin-plugins, add diff checker on…
antoooks Aug 27, 2023
d78fe66
add commentaries, change command order on presubmit-check
antoooks Aug 30, 2023
8f28349
add go mod tidy before workspace sync
antoooks Aug 31, 2023
a90c19d
Edit commentaries and ensure pluginator logic does not create diff
antoooks Sep 16, 2023
9e101ee
use doGoMod.sh forr go mod tidy
antoooks Sep 18, 2023
d91ea65
separate generate plugins and check diff step
antoooks Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ verify-kustomize-repo: \
.PHONY: prow-presubmit-check
prow-presubmit-check: \
install-tools \
workspace-sync \
generate-kustomize-builtin-plugins \
test-unit-kustomize-plugins \
test-go-mod \
build-non-plugin-all \
Expand Down Expand Up @@ -181,7 +183,12 @@ test-examples-kustomize-against-HEAD: $(MYGOBIN)/kustomize $(MYGOBIN)/mdrip
test-examples-kustomize-against-latest-release: $(MYGOBIN)/mdrip
./hack/testExamplesAgainstKustomize.sh v5@$(LATEST_RELEASE)


# Pushes dependencies in the go.work file back to go.mod files of each workspace module.
.PHONY: workspace-sync
workspace sync:
Copy link
Member

Choose a reason for hiding this comment

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

Do you have any specific reason to remove the hyphen here?
I think it is better to use a unified name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No issue here, yes we can go with unified name

go mod tidy
antoooks marked this conversation as resolved.
Show resolved Hide resolved
go work sync
antoooks marked this conversation as resolved.
Show resolved Hide resolved

# --- Cleanup targets ---
.PHONY: clean
clean: clean-kustomize-external-go-plugin uninstall-tools
Expand Down
23 changes: 21 additions & 2 deletions Makefile-plugins.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,28 @@ $(pGen)/%.go: $(MYGOBIN)/pluginator $(MYGOBIN)/goimports
$(MYGOBIN)/goimports -w $*.go \
)

# Target is for debugging.
# Generate builtin plugins
.PHONY: generate-kustomize-builtin-plugins
generate-kustomize-builtin-plugins: $(builtinplugins)
generate-kustomize-builtin-plugins: $(builtplugins)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I think it is better to separate a step of generate and a step of diff check.
Because generate-kustomize-builtin-plugins will be used in developing built-in plugins. This command may error when developing plugins before exec git commit.
I think it better to separate them there, It will be useful for developing plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. Do you know if diff check will be called anywhere else? Since the context is strictly limited to builtin plugins maybe I should use builtin-plugins no diff as target name instead

Copy link
Member

Choose a reason for hiding this comment

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

I imagine making a new builtin-plugins diff command, and that makes called from here.
I'll leave it to you to figure out how to do it.

for plugin in $(abspath $(wildcard $(pSrc)/*)); do \
echo "generating $${plugin} ..."; \
set -e; \
cd $${plugin}; \
go generate pluginator .; \
done; \
cd ../../../; \
make no-diff \

# Check for diff, if diff is found, throw error code 1
.PHONY: no-diff
no-diff: $(builtplugins)
for file in $(abspath $(builtinplugins)); do \
echo "Checking for diff... $${file}" ; \
set -e ; \
if [ "`git diff $${file} | wc -c`" -gt 0 ]; then\
echo "Error(1): diff found on $${file}"; exit 1; \
fi \
done

.PHONY: build-kustomize-external-go-plugin
build-kustomize-external-go-plugin:
Expand Down
5 changes: 3 additions & 2 deletions cmd/pluginator/internal/builtinplugin/builtinplugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ func ConvertToBuiltInPlugin() (retErr error) {
w.write(
fmt.Sprintf(
"// pluginator %s\n", provenance.GetProvenance().Short()))
w.write("\n")
w.write("package " + packageForGeneratedCode)

pType := unknown
Expand All @@ -73,6 +72,8 @@ func ConvertToBuiltInPlugin() (retErr error) {
continue
}
if strings.HasPrefix(l, "var "+konfig.PluginSymbol+" plugin") {
// Hack to skip leading new line
scanner.Scan()
continue
}
if strings.Contains(l, " Transform(") {
Expand All @@ -93,7 +94,7 @@ func ConvertToBuiltInPlugin() (retErr error) {
}
w.write("")
w.write("func New" + root + "Plugin() resmap." + pType.String() + "Plugin {")
w.write(" return &" + root + "Plugin{}")
w.write(" return &" + root + "Plugin{}")
w.write("}")

return nil
Expand Down