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

auth_soft_fail needed for public images when agent is configured with auth #11190

Merged
merged 8 commits into from
Oct 6, 2021
Merged

auth_soft_fail needed for public images when agent is configured with auth #11190

merged 8 commits into from
Oct 6, 2021

Conversation

shantanugadgil
Copy link
Contributor

auth_soft_fail needed for public image when agent is configured with auth

@lgfa29
Copy link
Contributor

lgfa29 commented Sep 16, 2021

Thanks @shantanugadgil 🙂

I don't know why we space out the config block. Maybe we can set it to be more compact like this? What do you think?

config {
  image = "redis:3.2"
  ports = ["db"]
                        
  auth_soft_fail = true
}

@lgfa29 lgfa29 self-requested a review September 16, 2021 21:50
@shantanugadgil
Copy link
Contributor Author

Thanks @shantanugadgil slightly_smiling_face

I don't know why we space out the config block. Maybe we can set it to be more compact like this? What do you think?

config {
  image = "redis:3.2"
  ports = ["db"]
                        
  auth_soft_fail = true
}

I am fine with the above change you have proposed.

or even something like:

config {
  image          = "redis:3.2"
  ports          = ["db"]
  auth_soft_fail = true
}

Side note, after some grepping, I discovered there could be more "example" files in code which would need this fix though.

@lgfa29
Copy link
Contributor

lgfa29 commented Sep 23, 2021

Thanks @shantanugadgil slightly_smiling_face
I don't know why we space out the config block. Maybe we can set it to be more compact like this? What do you think?

config {
  image = "redis:3.2"
  ports = ["db"]
                        
  auth_soft_fail = true
}

I am fine with the above change you have proposed.

or even something like:

config {
  image          = "redis:3.2"
  ports          = ["db"]
  auth_soft_fail = true
}

Yeah, I think that works as well.

Side note, after some grepping, I discovered there could be more "example" files in code which would need this fix though.

Good point. I think connect.nomad and connect-short.nomad would need them as well. Any other reference to the example job I think would OK since command/assets is the path used by the nomad job init command.

Where else were you thinking about changing?

@vercel vercel bot temporarily deployed to Preview – nomad September 23, 2021 06:01 Inactive
@shantanugadgil
Copy link
Contributor Author

@lgfa29 I have now modified all .nomad files inside command/assets/.

@lgfa29
Copy link
Contributor

lgfa29 commented Oct 5, 2021

Thanks @shantanugadgil!

I pushed a commit to simplify the comment text a little. I also removed the second comment from the Connect example since it's already set in the first task.

Does the new comment text still makes sense?

@lgfa29 lgfa29 added this to the 1.2.0 milestone Oct 5, 2021
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

I forgot to actually accept and merge.

Thank you @shantanugadgil!

@lgfa29 lgfa29 merged commit 20b44d7 into hashicorp:main Oct 6, 2021
@shantanugadgil
Copy link
Contributor Author

Thanks. the comments seem fine. Punctuation can be bettered, but having that option in there is more important! 👍

@shantanugadgil
Copy link
Contributor Author

@lgfa29 this change doesn't appear even with the latest GA 1.3.1. I am not sure why the assets are not being used correctly! :(

# nomad --version
Nomad v1.3.1 (2b054e38e91af964d1235faa98c286ca3f527e56)

# nomad job init -short 
Example job file written to example.nomad

# vim example.nomad

---
 11     task "redis" {
 12       driver = "docker"
 13 
 14       config {
 15         image = "redis:3.2"
 16 
 17         ports = ["db"]
 18       }
 19 
---


@lgfa29
Copy link
Contributor

lgfa29 commented May 27, 2022

Oh that's weird...I'm not sure why the generate file was not updated during the release process. We call make prerelease, which calls generate-all, which then calls generate-examples, which was supposed to update the command/job_init.bindata_assetfs.go file with the new binary data.

I think that, because the file already exists, make doesn't try to recreate 🤔

I will investigate this further, thanks for let me know!

@shantanugadgil
Copy link
Contributor Author

Just wondering if you were able to determine the issue with the assets.

@lgfa29
Copy link
Contributor

lgfa29 commented Jul 15, 2022

Just wondering if you were able to determine the issue with the assets.

Sorry, I missed this 😅

I didn't find the root cause, but manually fixed. I should be there in 1.3.2.

@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 Nov 12, 2022
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