Skip to content

Move magician into tools#13939

Closed
trodge wants to merge 4 commits intoGoogleCloudPlatform:mainfrom
trodge:membership-tools
Closed

Move magician into tools#13939
trodge wants to merge 4 commits intoGoogleCloudPlatform:mainfrom
trodge:membership-tools

Conversation

@trodge
Copy link
Copy Markdown
Contributor

@trodge trodge commented May 13, 2025

This will allow membership data to be shared with the good build approver.

Commands used:

mv .ci/magician tools
for i in $(grep -rl "\.ci\/magician"); do sed -i "s/\.ci\/magician/tools\/magician/" $i; done
# fix relative path in .ci/scripts/go-plus/magician/exec.sh
cd tools/magician
# fix path to issue-labeler in go.mod
go mod tidy

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.


@modular-magician
Copy link
Copy Markdown
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@trodge trodge force-pushed the membership-tools branch from 7509361 to 7f69b69 Compare June 2, 2025 02:29
@modular-magician
Copy link
Copy Markdown
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@trodge
Copy link
Copy Markdown
Contributor Author

trodge commented Jun 2, 2025

@modular-magician reassign-reviewer

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 2, 2025

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

Googlers: For automatic test runs see go/terraform-auto-test-runs.

@ScottSuarez, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

@github-actions github-actions bot requested a review from zli82016 June 2, 2025 17:19
@zli82016
Copy link
Copy Markdown
Member

zli82016 commented Jun 2, 2025

@modular-magician reassign-reviewer

@github-actions github-actions bot requested review from ScottSuarez and removed request for zli82016 June 2, 2025 18:04
Copy link
Copy Markdown
Contributor

@ScottSuarez ScottSuarez left a comment

Choose a reason for hiding this comment

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

Do we want to migrate membership checker to use this library so aren't duplicating code?

}
}

func (gh *Client) GetUserType(user string) UserType {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

undefined: ClientcompilerUndeclaredName

Should this define a client interface that has this function?

@@ -0,0 +1,8 @@
module github.com/trodge/magic-modules/membership-tools/tools/github-membership

go 1.25
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
go 1.25
go 1.23

consistency with other packages

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@trodge
Copy link
Copy Markdown
Contributor Author

trodge commented Jun 2, 2025

Do we want to migrate membership checker to use this library so aren't duplicating code?

Yes, but I want to copy it first so that importing is easier.

@trodge
Copy link
Copy Markdown
Contributor Author

trodge commented Jun 2, 2025

It's now occurring to me that this approach won't work because I'll need to import the Client methods used here, and they can't be imported because they're in the .ci directory. I think a better solution is going to be to move the entire magician into tools so that it can be imported from anywhere, including the good build approver.

@trodge trodge changed the title Add github membership data to tools Move magician into tools Jun 2, 2025
@modular-magician
Copy link
Copy Markdown
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@modular-magician
Copy link
Copy Markdown
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@ScottSuarez
Copy link
Copy Markdown
Contributor

It's now occurring to me that this approach won't work because I'll need to import the Client methods used here, and they can't be imported because they're in the .ci directory. I think a better solution is going to be to move the entire magician into tools so that it can be imported from anywhere, including the good build approver.

I'm not sure I understand the purpose of moving the code? We can just import it where it lives today, no?

@trodge
Copy link
Copy Markdown
Contributor Author

trodge commented Jun 3, 2025

It's now occurring to me that this approach won't work because I'll need to import the Client methods used here, and they can't be imported because they're in the .ci directory. I think a better solution is going to be to move the entire magician into tools so that it can be imported from anywhere, including the good build approver.

I'm not sure I understand the purpose of moving the code? We can just import it where it lives today, no?

Packages from paths containing a leading . cannot be imported. https://pkg.go.dev/golang.org/x/mod/module#CheckImportPath

@ScottSuarez ScottSuarez requested a review from melinath June 3, 2025 20:08
@ScottSuarez
Copy link
Copy Markdown
Contributor

It's now occurring to me that this approach won't work because I'll need to import the Client methods used here, and they can't be imported because they're in the .ci directory. I think a better solution is going to be to move the entire magician into tools so that it can be imported from anywhere, including the good build approver.

I'm not sure I understand the purpose of moving the code? We can just import it where it lives today, no?

Packages from paths containing a leading . cannot be imported. pkg.go.dev/golang.org/x/mod/module#CheckImportPath

I'm okay with this philosophically but would like Stephen to sign off as well just to make sure I'm not missing anything !

@melinath
Copy link
Copy Markdown
Member

melinath commented Jun 3, 2025

I'm initially hesitant to make this large of a change when the stated purpose is pretty narrow. It feels like we ought to be able to only move the membership code + client layer code, rather than needing to move everything.

There's also a security question: if we're moving any code from .ci to tools, we should take a moment to consider whether we should automatically consider changes to tools unsafe, similar to .ci.

That being said - checking membership is now significantly easier than it was previously so perhaps we don't need to share code with the GBA at all?

@melinath
Copy link
Copy Markdown
Member

melinath commented Jun 3, 2025

As discussed - switching to a yaml file for membership / vacation data, will pull that into the GBA as needed. We'll implement the membership check in the GBA separately instead of sharing code.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 6, 2025

@ScottSuarez This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

@github-actions
Copy link
Copy Markdown

@GoogleCloudPlatform/terraform-team @ScottSuarez This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

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.

5 participants