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.

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

Add new makefile targets for go mod verification #3550

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

razo7
Copy link
Collaborator

@razo7 razo7 commented Apr 30, 2024

Which issue this PR addresses:

None. It is a small enhancement. Duplicate of #3492.

What this PR does / why we need it:

Run 'go mod' functions to search for tidy, and vendor package changes and then verify it.

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

Yes, in the second commit

How do you know this will function as expected in production?

Run 'go mod' functions to search for tidy, vendor changes and verify it
Update makefile and one doc file docomentation with the change of go-verify over using vendor and tidy
@razo7
Copy link
Collaborator Author

razo7 commented Apr 30, 2024

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -167,7 +170,7 @@ run-portal:
go run -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=$(VERSION)" ./cmd/aro portal

build-portal:
cd portal/v2 && npm install && npm run build
cd portal/v1 && npm install && npm run build && cd ../v2 && npm install && npm run build
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this line? It was deleted as part of PR #3465 that removed portal/v1

Copy link
Collaborator

@anshulvermapatel anshulvermapatel left a comment

Choose a reason for hiding this comment

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

Everything looks good to me, except what @tiguelu already pointed about. Once thats changed. LGTM

Copy link
Collaborator

@mociarain mociarain left a comment

Choose a reason for hiding this comment

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

Nice change. Just a few questions

@@ -266,9 +269,22 @@ admin.kubeconfig:
aks.kubeconfig:
hack/get-admin-aks-kubeconfig.sh

.PHONY: go-tidy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why each individual PHONY and not added to the overall PHONY at the bottom of the file?

@@ -18,6 +18,9 @@ GATEKEEPER_VERSION = v3.10.0
GATEKEEPER_IMAGE ?= ${RP_IMAGE_ACR}.azurecr.io/gatekeeper:$(GATEKEEPER_VERSION)
GOTESTSUM = gotest.tools/gotestsum@v1.11.0

# Golang version go mod tidy compatibility
GOLANG_VERSION ?= 1.20
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we specify 1.20.12? (Still finding my feet with Go, I have no idea how strict it is with versions)

For reference: #3548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants