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

chore: use workspaces to make api changes easier #635

Merged
merged 7 commits into from
May 13, 2024

Conversation

bacherfl
Copy link
Contributor

@bacherfl bacherfl commented May 8, 2024

This PR introduces the use of go workspaces. Before this change, anything that involves a change or addition in the apis submodule required this to be split into multiple PRs (one for the api change, then a follow up to adapt the code in the controller/webhook). A similar approach is also used in https://github.com/open-feature/flagd, so I think we could also make use of that here

Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl bacherfl marked this pull request as ready for review May 13, 2024 05:28
@bacherfl bacherfl requested a review from a team as a code owner May 13, 2024 05:28
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Great idea!

One question: Is there any CI (particularly the release CI) that needs to be adapted for this to work? Do those flows need to init the workspace?

@bacherfl
Copy link
Contributor Author

bacherfl commented May 13, 2024

Great idea!

One question: Is there any CI (particularly the release CI) that needs to be adapted for this to work? Do those flows need to init the workspace?

Looking at the CI I believe that the workspace might need to be initialised here:

although i'm not sure if the go mod tidy and make controller-gen is actually needed - do you have some background information on why this was added here?

Edit: also I noticed the workspace init was missing before the test execution, so i have added it here as well

Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@toddbaert
Copy link
Member

Great idea!
One question: Is there any CI (particularly the release CI) that needs to be adapted for this to work? Do those flows need to init the workspace?

Looking at the CI I believe that the workspace might need to be initialised here:

although i'm not sure if the go mod tidy and make controller-gen is actually needed - do you have some background information on why this was added here?

Edit: also I noticed the workspace init was missing before the test execution, so i have added it here as well

Based on blame, it's been this way for 2 years: d4abda1

It seems like this is(was?) necessary before running ... make helm-package and ... make release-manifests on the next lines. I don't see any Go dependency management or anything like that in the Makefile, so I guess that's why it's needed here? Does that make sense?

@toddbaert
Copy link
Member

Merging this now... we're releasing today so we'll know if it works 😅

@toddbaert toddbaert merged commit 0479540 into open-feature:main May 13, 2024
13 checks passed
@github-actions github-actions bot mentioned this pull request May 13, 2024
@toddbaert
Copy link
Member

Worked fine with the workspace init. I think we could move this into the makefile to make things more self-contained, but at least everything works as is.

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.

4 participants