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

Added support to configure the agent against deployment pool #1230

Merged
merged 6 commits into from
Oct 11, 2017
Merged

Added support to configure the agent against deployment pool #1230

merged 6 commits into from
Oct 11, 2017

Conversation

chshrikh
Copy link
Contributor

@chshrikh chshrikh commented Oct 6, 2017

if user provide the new flag --deploymentpool, user will be able to configure the agent against deployment pool

TingluoHuang
TingluoHuang reviewed Oct 8, 2017
View reviewed changes
src/Microsoft.VisualStudio.Services.Agent/AgentServer.cs
{
CheckConnection();
return _taskAgentClient.GetAgentPoolsAsync(agentPoolName);
return _taskAgentClient.GetAgentPoolsAsync(agentPoolName, poolType: poolType);
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we just change the server to return both pool type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently we dont have any consumer which requires all type of pool. I would like to keep it tightly bound with type.

@TingluoHuang
Copy link
Contributor

Few questions.

  1. The current deployment group stuff is trying to hide the agent pool concept from end user. doesn't like build/release, deployment group user configure the agent against an deployment group. right?
  2. This change means when user configure agent (build/release/deploymentgroup), he can always just configure against the agent pool, right?
  3. Are we going to kill the concept of configure the agent against a deployment group?
  4. Will this introduce confusion, since there are 2 way to configure the agent for a deployment group?

@chshrikh
Copy link
Contributor Author

@TingluoHuang
Deployment groups deals with two experience:

  1. A project user who creates DG (and backing DP automatically) which is used in RD. This experience is at Project level.
  2. A machine admin (like a MSIT/BGIT admin) who manages hardware at Collection/Application level and lets Project level user to create DG using the shared hardware.

So far we had support for #1. With Machine sharing feature we are lighting up support for #2. Both the experiences will co-exist

  1. The current deployment group stuff is trying to hide the agent pool concept from end user. doesn't like build/release, deployment group user configure the agent against an deployment group. right?

    >>	Yes. This is the scenario #1 I mentioned above.

  2. This change means when user configure agent (build/release/deploymentgroup), he can always just configure against the agent pool, right?

    >> This is a new support to enable scenario #2 (Machine sharing).

  3. Are we going to kill the concept of configure the agent against a deployment group?

    >> Scenario #1 will continue since we don’t have to expose pools to Project level users.

  4. Will this introduce confusion, since there are 2 way to configure the agent for a deployment group?

    >> No. 
   Project level user will continue to work with DG at project level. The user is not required to learn Pool as a concept unless there is a machine sharing scenario involved. 
   Admins at Collection level will be able to register at Pool and they operate at Pools, a concept that they are familiar with.

TingluoHuang
TingluoHuang reviewed Oct 10, 2017
View reviewed changes
src/Agent.Listener/Configuration/ConfigurationManager.cs Outdated
string agentType = command.DeploymentGroup
? Constants.Agent.AgentConfigurationProvider.DeploymentAgentConfiguration
: Constants.Agent.AgentConfigurationProvider.BuildReleasesAgentConfiguration;
string agentType = Constants.Agent.AgentConfigurationProvider.BuildReleasesAgentConfiguration;
Copy link
Contributor

Choose a reason for hiding this comment

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

string agentType;
if()
{agentType = xxx}
elseif()
{agentType = xxx}
else
{agentType = xxx}

TingluoHuang
TingluoHuang reviewed Oct 10, 2017
View reviewed changes
src/Agent.Listener/Configuration/ConfigurationProvider.cs Outdated

public override async Task GetPoolId(AgentSettings agentSettings, CommandSettings command)
{
int poolId = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need this?

TingluoHuang
TingluoHuang reviewed Oct 10, 2017
View reviewed changes
src/Agent.Listener/Configuration/ConfigurationProvider.cs Outdated
agentSettings.PoolId = agentPool.Id;
}

Trace.Info($"PoolId for deployment pool '{poolName}' is '{poolId}'.");
Copy link
Contributor

Choose a reason for hiding this comment

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

poolid is always 0...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

TingluoHuang
TingluoHuang reviewed Oct 11, 2017
View reviewed changes
src/Agent.Listener/Configuration/ConfigurationProvider.cs
@@ -54,7 +54,7 @@ public void GetServerUrl(AgentSettings agentSettings, CommandSettings command)
agentSettings.ServerUrl = command.GetUrl();
}

public async Task GetPoolId(AgentSettings agentSettings, CommandSettings command)
public virtual async Task GetPoolId(AgentSettings agentSettings, CommandSettings command)
Copy link
Contributor

Choose a reason for hiding this comment

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

this method looks weird.
can you just do GetPoolIdAsync() inline, like what you had in the overwrite one.

Copy link
Contributor

Choose a reason for hiding this comment

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

GetPoolIdAsync() is a private method only called by one method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will have only one method GetPoolId(). will remove GetPoolIdAsync

@TingluoHuang
Copy link
Contributor

@chshrikh I created an .49 version of vss-api-core, you can use.

@TingluoHuang
Copy link
Contributor

@chshrikh please check the last comment i had, looks good overall.

@chshrikh chshrikh merged commit 7abd52e into master Oct 11, 2017
@chshrikh chshrikh deleted the users/chshrikh/AddSupportToDeploymentPool branch October 11, 2017 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TingluoHuang TingluoHuang TingluoHuang left review comments

@kmkumaran kmkumaran kmkumaran approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chshrikh @TingluoHuang @kmkumaran