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

[JENKINS-49661] use "agent" for Symbol #3304

Merged
merged 1 commit into from Feb 23, 2018

Conversation

6 participants
@ndeloof
Copy link
Member

commented Feb 20, 2018

See JENKINS-49661.

Proposed changelog entries

  • support "agent" as a symbol for permanent agents

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change).
  • Appropriate autotests or explanation to why this change has no tests
    no need for test here

Desired reviewers

@oleg-nenashev
@daniel-beck

@ndeloof ndeloof requested review from daniel-beck and oleg-nenashev Feb 20, 2018

@@ -66,7 +66,7 @@ public DumbSlave(@Nonnull String name, String remoteFS, ComputerLauncher launche
}

@Extension @Symbol({"dumb",
"slave"/*because this is in effect the canonical slave type*/})
"slave", "agent" /*because this is in effect the canonical slave type*/})

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 20, 2018

Member

not sure the comment still makes sense

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Feb 20, 2018

Contributor

This doesn't create a conflict in the Declarative Pipeline with its use of agent or with other locations where "agent" is already used?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

Could we please avoid referencing Dumb Slaves in changelogs? :trollface:

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

I would argue it should be permanentAgent like in UI

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

@oleg-nenashev oleg-nenashev changed the title [JENKINS-42816] use "agent" for Symbol [JENKINS-49661] use "agent" for Symbol Feb 20, 2018

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

The sad part is that this change we're amending was originally merged into 2.2, when slave was already obsolete 😢

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

Blocked the PR in order to discuss the naming, if @jenkinsci/code-reviewers agree that "agent" is fine, so be it

@rtyler

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

@oleg-nenashev I don't have any context for where these symbols show up, except in the Configuration as Code plugin, and I think agent is just fine. There's no UI for configuring in-permanent agents really (I know the Cloud API stuff exists, but that's all in a totally different place)

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

Actually permanent agents are on the front page when you want to create an agent:

screen shot 2018-02-21 at 00 28 52

@rtyler

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

@oleg-nenashev I understand that, but there's no "in-permanent agent" or otherwise dynamic agent configuration which occurs on that interface. And that's the only place where "Permanent Agent" seems to be used (from my experience).

My point is that the only place I have seen this @Symbol manifest in a user interface, is through the Configuration as Code plugin. Because of that ,I suggest simply using agent for brevity.

@jglick

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

It is commonplace for there to be Slaves which are not DumbSlaves, if you are using Clouds. But these tend to be precisely those nodes you do not configure individually; you would be configuring something else. So plain agent seems reasonable to me.

dismissing for now

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

IIUC there are 3 +1 votes and my +0.5 (or so).
Other types with fixed agents will need to take it into account, but OK.

I will put it to ready-for-merge and merge into the weekly unless somebody votes again

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

OK, merging

@oleg-nenashev oleg-nenashev merged commit 72d80b4 into master Feb 23, 2018

1 check passed

continuous-integration/jenkins/branch This commit looks good
Details

@oleg-nenashev oleg-nenashev deleted the ndeloof-JENKINS-42816 branch Jun 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.