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

Shorten CLI identifiers #675

Merged
merged 5 commits into from
Jan 19, 2016
Merged

Shorten CLI identifiers #675

merged 5 commits into from
Jan 19, 2016

Conversation

iverberk
Copy link
Contributor

  • Truncate all UUID identifiers to eight characters by default
  • Refactor the node identifier to an auto-generated UUID
  • Created and updated tests and mocks

* Truncate all UUID identifiers to eight characters by default
* Refactor the node identifier to an auto-generated UUID
* Created and updated tests and mocks
@iverberk
Copy link
Contributor Author

Not sure what is going on with the tests failing. They run without errors on my local machine. Will take a look at them tomorrow. Would be nice to know if this is happening locally to other people as well.

* full-id argument is now called verbose to be more future-proof
* constants for identifier length are a little more concise
@@ -450,6 +450,11 @@ func (c *Client) setupNode() error {
node = &structs.Node{}
c.config.Node = node
}
id, err := c.nodeID()
Copy link
Contributor

Choose a reason for hiding this comment

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

can you do:

// Generate an ID for the node
var err error
node.id, err := c.nodeID()
if err != nil {...}

@dadgar
Copy link
Contributor

dadgar commented Jan 19, 2016

LGTM! Sorry this is a bit tedious but can you update the documentation on these pages: https://www.nomadproject.io/docs/commands/agent-info.html

@iverberk
Copy link
Contributor Author

Thanks for the review! I updated the code according to your comments. The website documentation has also been updated, not tedious at all, just part of maintaining a solid product.

dadgar added a commit that referenced this pull request Jan 19, 2016
@dadgar dadgar merged commit 0d29e30 into hashicorp:master Jan 19, 2016
@dadgar
Copy link
Contributor

dadgar commented Jan 19, 2016

Awesome work! Thanks @iverberk

@iverberk
Copy link
Contributor Author

Sweet, thanks! :-) Maybe also good to mention in the changelog that the node id can no longer be specified through the configuration (backwards incompatible)?

@dadgar
Copy link
Contributor

dadgar commented Jan 20, 2016

Yeah I threw it under the "BACKWORDS INCOMPATIBLE" section. We will have to
put together an upgrade guide for 0.2->0.3

On Tue, Jan 19, 2016 at 3:25 PM, iverberk [email protected] wrote:

Sweet, thanks! :-) Maybe also good to mention in the changelog that the
node id can no longer be specified through the configuration (backwards
incompatible)?


Reply to this email directly or view it on GitHub
#675 (comment).

VladRassokhin referenced this pull request Feb 9, 2016
ketzacoatl added a commit to ketzacoatl/nomad that referenced this pull request Apr 8, 2016
Specifically, see https://github.com/hashicorp/nomad/blob/master/CHANGELOG.md#030

> Node ID is no longer specifiable. For users who have set a custom Node ID, the node should be drained before Nomad is updated and the data_dir should be deleted before starting for the first time [hashicorpGH-675]
dadgar added a commit that referenced this pull request Apr 8, 2016
Dropped `node_id` from docs, per #675
@github-actions
Copy link

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 30, 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.

2 participants