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

Jump to bottom

refactor: change './bin' to 'LOCALBIN' #6655

Merged
merged 4 commits into from
Jan 12, 2024
Merged

refactor: change './bin' to 'LOCALBIN' #6655

merged 4 commits into from
Jan 12, 2024

Conversation

lunarwhite
Copy link
Contributor

Description of the change:

avoid hardcoded tool binary variables OPERATOR_SDK and OPM.

Motivation for the change:

fixes #6637

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2024
@lunarwhite lunarwhite marked this pull request as ready for review January 8, 2024 06:24
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2024
everettraven
everettraven reviewed Jan 8, 2024
View reviewed changes
internal/plugins/manifests/v2/init.go Outdated
Comment on lines 185 to 188
# Location to install dependencies to
LOCALBIN ?= $(shell pwd)/bin
$(LOCALBIN):
mkdir -p $(LOCALBIN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does LOCALBIN already exist in the makefile that we are going to be adding this information to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I'll update.

everettraven
everettraven requested changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR @lunarwhite ! Would you mind regenerating the testdata with make generate and pushing up the changes?

@lunarwhite
refactor: change './bin' to 'LOCALBIN'
04f5bef 
avoid hardcoded tool binary variables

Signed-off-by: GitHub <noreply@github.com>
@lunarwhite
auto bump Copyright to 2024
efab731 
'make generate' also change every Copyright 2023 to 2024, add a
separate commit for reviewer to review easily

Signed-off-by: GitHub <noreply@github.com>
@lunarwhite
Copy link
Contributor Author

Hi @everettraven, thanks for reviewing. At this time, make generate will also auto change Copyright 2023 to 2024. So I separate logic into two commits, to make reviewing process easier.

Adding label to squash commits:

/label tide/merge-method-squash

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 8, 2024
everettraven
everettraven approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Thanks @lunarwhite ! Looks good to me!

This might need a rebase based on a recently merged PR

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jan 9, 2024

New changes are detected. LGTM label has been removed.

@lunarwhite
Copy link
Contributor Author

Hi @everettraven, it seems that workflow jobs require maintainer's approval.

@lunarwhite
fix dead link error
82ed477 
Signed-off-by: GitHub <noreply@github.com>
@lunarwhite
Copy link
Contributor Author

Last commit will fix "sanity / docs (pull_request) Failing after 2m"

Running ["ScriptCheck", "LinkCheck", "ImageCheck"] on ["/target"] on *.html... 


Checking 777 external links...
- /target/docs/best-practices/observability-best-practices/index.html
  *  External link https://github.com/operator-framework/operator-sdk/tree/master/testdata/go/v4-alpha/monitoring/memcached-operator/monitoring failed: 404 No error
Ran on 170 files!

HTML-Proofer found 1 failure!


sdk-html
exiting @ Thu Jan 11 19:56:04 UTC 2024
make: *** [Makefile:156: test-docs] Error 1
Error: Process completed with exit code 2.

It's a little weird as I didn't touch this kind of file. Anyway, it failed mainly because

operator-sdk/changelog/generated/v1.33.0.md

Line 6 in c557c54

- **Breaking change**: (go/v4-alpha): go/v4 is now stable and is the default version used when scaffolding a Go based operator. ([#6613](https://github.com/operator-framework/operator-sdk/pull/6613))

Now I believe it's ok to re-run jobs, sorry for the frequent requests.

@everettraven everettraven merged commit ce3a767 into operator-framework:master Jan 12, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@everettraven everettraven everettraven approved these changes

@anik120 anik120 Awaiting requested review from anik120

@joelanford joelanford Awaiting requested review from joelanford

Assignees

@everettraven everettraven

Labels
tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change ./bin to LOCALBIN
2 participants
@lunarwhite @everettraven