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

Allow client template config block to be parsed when using json config #24007

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

ncode
Copy link
Contributor

@ncode ncode commented Sep 19, 2024

cat ./debug/config.json
{
  "bind_addr": "0.0.0.0",
  "client": {
    "cni_config_dir": "/etc/nomad/cni.d",
    "cni_path": "/usr/libexec/cni",
    "enabled": true,
    "max_dynamic_port": 20010,
    "min_dynamic_port": 20000,
    "servers": [
      "nomad-server-001.internal"
    ],
    "template": {
      "wait": {
        "max": "15s",
        "min": "5s"
      },
      "wait_bounds": {
        "max": "15s",
        "min": "5s"
      }
    }
  },
  "consul": {
    "address": "consul.internal:8500"
  },
  "data_dir": "/opt/nomad/data",
  "datacenter": "dc1",
  "name": "nomad-client-001",
  "telemetry": {
    "collection_interval": "1s",
    "disable_hostname": true,
    "prometheus_metrics": true,
    "publish_allocation_metrics": true,
    "publish_node_metrics": true
  },
  "vault": {
    "address": "https://vault.services.internal",
    "enabled": true
  }
}

Btw I think the artifacts block will have the same problem

Fixes #24001

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @ncode! There should be tests in config_parse_test.go that will exercise this code. We should make sure this bug fix is covered by those tests.

Also, can you run make cl to add a changelog item for the bug fix?

@tgross tgross self-assigned this Sep 27, 2024
@ncode
Copy link
Contributor Author

ncode commented Sep 30, 2024

@tgross cool, I'm going to work on that today!

@ncode ncode changed the title Allow client template config block to be parsed when using json config WIP: Allow client template config block to be parsed when using json config Oct 1, 2024
@ncode ncode changed the title WIP: Allow client template config block to be parsed when using json config Allow client template config block to be parsed when using json config Oct 1, 2024
@ncode ncode requested a review from tgross October 1, 2024 09:36
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @ncode! Test looks great but we need to get CI passing before we can merge. If you hclfmt that one file, that should allow checks to pass and tests to run.

- Adds tests
- Adds sample test data for parsing hcl and json
- Adds changelog
@ncode ncode requested a review from tgross October 1, 2024 19:21
@tgross tgross added backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/1.8.x backport to 1.8.x release line labels Oct 1, 2024
@tgross tgross added this to the 1.9.0 milestone Oct 1, 2024
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

This will ship in the upcoming Nomad 1.9, with backports to Nomad Enterprise. Thanks again @ncode!

@tgross tgross merged commit 4a74fda into hashicorp:main Oct 1, 2024
24 checks passed
tgross added a commit that referenced this pull request Oct 1, 2024
In #24007 we merged new HCL files but they were missing copywrite headers
because the scan didn't run on this PR for some reason. I've already backported
this to the Enterprise branches.
tgross added a commit that referenced this pull request Oct 1, 2024
In #24007 we merged new HCL files but they were missing copywrite headers
because the scan didn't run on this PR for some reason. I've already backported
this to the Enterprise branches.
@ncode ncode deleted the juliano/json_config_fix branch October 7, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/1.8.x backport to 1.8.x release line theme/template type/bug
Projects
Development

Successfully merging this pull request may close these issues.

Nomad unable to parse template client parameters when using json config format
2 participants