Skip to content

PIPE-1084: remove org as stack parameter#609

Merged
zhming0 merged 1 commit into
mainfrom
ming/pipe-1084
Jun 5, 2025
Merged

PIPE-1084: remove org as stack parameter#609
zhming0 merged 1 commit into
mainfrom
ming/pipe-1084

Conversation

@zhming0
Copy link
Copy Markdown
Contributor

@zhming0 zhming0 commented Jun 4, 2025

Org has been an unused parameter in the stack but it's marked as required.

This PR remove this params for good. There are various places in test setup needing it, a AgentTokenClient has been introduced so we can derive the Org from agent token.

@zhming0 zhming0 requested review from a team, DrJosh9000 and sj26 June 4, 2025 04:25
@zhming0 zhming0 requested a review from a team as a code owner June 4, 2025 04:25
@zhming0 zhming0 force-pushed the ming/pipe-1084 branch 3 times, most recently from 73602d4 to f9bbfee Compare June 4, 2025 04:41
Comment thread DEVELOPMENT.md
Comment thread DEVELOPMENT.md
Comment thread cmd/controller/controller.go Outdated
Comment thread api/agent_token_client.go

// This is a special agent client: it can only be used to query the GET /token API.
// Meaning for this client to function, there isn't a need for a cluster id nor queue id.
func NewAgentTokenClient(token, endpoint string) (*AgentTokenClient, error) {
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.

Should we combine this with AgentClient later on?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That was my plan but AgentClient needs org, cluster to function. On a quick glance it doesn't seem quite obvious to me on how to combine these (nor on the benefit), so I kept them separate for now.

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.

Well, you showed that org is unnecessary. I'm thinking that the work AgentTokenClient does could be part of "AgentClient connection setup", so that NewAgentClient no longer needs the cluster ID as an arg. That leaves the queue parameter, which isn't needed for AgentTokenClient, but the arg could be passed an empty string for clients that don't need to get scheduled jobs, etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean now. I agree.

@zhming0 zhming0 enabled auto-merge June 5, 2025 03:36
@zhming0 zhming0 merged commit 21b843c into main Jun 5, 2025
1 check passed
@zhming0 zhming0 deleted the ming/pipe-1084 branch June 5, 2025 03:44
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.

3 participants