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

Add env vars for dc, region #2507

Merged
merged 12 commits into from
May 4, 2017
Merged

Add env vars for dc, region #2507

merged 12 commits into from
May 4, 2017

Conversation

qkate
Copy link
Contributor

@qkate qkate commented Mar 30, 2017

Closes #2183

  • adds env vars for datacenter and region to job env
  • makes them both available for interpolation from within jobs

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

👍 Add a line in CHANGELOG.md (either in this PR or directly to master after merging)

@jippi
Copy link
Contributor

jippi commented Mar 31, 2017

Nice @qkate !

@@ -708,6 +708,9 @@ type Node struct {
// Datacenter for this node
Datacenter string

// Region for this node
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey lets actually remove this. It is better to keep this normalized as we will not have a field string stored in memory for every node that is know by virtue of the region you are talking to.

@@ -278,6 +278,8 @@ func GetTaskEnv(taskDir *allocdir.TaskDir, node *structs.Node,
env := env.NewTaskEnvironment(node).
SetTaskMeta(alloc.Job.CombinedTaskMeta(alloc.TaskGroup, task.Name)).
SetJobName(alloc.Job.Name).
SetDatacenterName(node.Datacenter).
SetRegionName(node.Region).
Copy link
Contributor

Choose a reason for hiding this comment

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

conf.Region

@@ -61,6 +61,14 @@ environment variables.
<td>The job's name</td>
</tr>
<tr>
<td>`NOMAD_DC`</td>
<td>The datacenter in which the job is running.</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Pedantic, but these aren't sentences, so we should not have a period at the end 😄

@qkate
Copy link
Contributor Author

qkate commented Apr 5, 2017

@sethvargo -- roger that, I removed the periods!

@@ -227,6 +242,7 @@ func (t *TaskEnvironment) Build() *TaskEnvironment {
// Set up the node values.
t.NodeValues[nodeIdKey] = t.Node.ID
t.NodeValues[nodeDcKey] = t.Node.Datacenter
t.NodeValues[nodeRegionKey] = t.Node.Region
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dadgar I'm guessing I should remove this line as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would change it to `t.NodeValues[nodeRegionKey] = t.Region

@@ -275,6 +275,7 @@ func (a *Agent) clientConfig() (*clientconfig.Config, error) {
// Setup the node
conf.Node = new(structs.Node)
conf.Node.Datacenter = a.config.Datacenter
conf.Node.Region = a.config.Region
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dadgar And should I remove this too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep

@@ -61,6 +61,14 @@ environment variables.
<td>The job's name</td>
</tr>
<tr>
<td>`NOMAD_DC`</td>
<td>The datacenter in which the job is running</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

job -> allocation

</tr>
<tr>
<td>`NOMAD_REGION`</td>
<td>The region in which the job is running</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor

@dadgar dadgar left a comment

Choose a reason for hiding this comment

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

@dadgar dadgar force-pushed the f-task-env-vars branch from 8c39d25 to 3642434 Compare May 3, 2017 22:14
@dadgar dadgar merged commit ec3aec5 into master May 4, 2017
@dadgar dadgar deleted the f-task-env-vars branch May 4, 2017 00:22
@wilkmaia
Copy link

wilkmaia commented May 4, 2017

@dadgar
Copy link
Contributor

dadgar commented May 4, 2017

@wilkmaia that is a somewhat flaky test so given all else passed and they are unrelated I think it is a red herring! But thanks for noticing!

@github-actions
Copy link

github-actions bot commented Apr 1, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants