I think we can overwritedisplayedTags here entirely. If using a provisioner key, none of the tags specified viatags will be used to match jobs to the provisioner. (In fact, I don't think you're evenallowed specify both--key and--tag)

I preferred to keep two different variables as otherwise it would require some other logic changes.

The tag variable is used to call the/provisionerdaemon/serve endpoint below, and using the same variable would mean changes here too, which I feel like we dont want.

wdyt ?

For the sake of correctness, I'd like to make this change.
Otherwise this may confuse future readers of the code.
However, let's do it as a separate follow-up PR.

review: ThisdisplayedTags var is merging both tags provided in the CLI and tags fetched from the provisionerKey.

Are there any sad-cases that we could be testing here?
Is there a test already (sorry, being lazy) which already checks that if a provisioner key is not defined then it will not display the tags?

We could add a test to validate what happen in case of theclient.GetProvisionerKey failing - I've just done it.

Otherwise the logic should not be impacted and the endpoint itself seems pretty much well tested.

I'm not really understanding the point ofNoProvisionerKeyFound. The purpose of this PR is to add changes to tags which are logged, but AFAICS you're not looking at the outputted logs? Does the provisioner fail to start in this case?

Yeah that's exactly it - I am open to the behavior we want to apply here but globally if we can not contact the backend / retrieve the key , I returned and error and did not continued to start the provisioner.

We can instead just output an error and continue as if nothing happened - but the log will be the same as of now (without the tags)

Cool, no need to change the behaviour - I was just confirming what the intent was for this test.

What is your intention here? To clear out the map if the provisioner key is set?

So globally, either we have no key and some tags (line 107 to 110) - either we have no key (line 112) and want to fill the displayedTags with the ones fetched inpkDetails.

That's why at first I was using the same map without allocating it , as we can't really be sure about the size before knowing in which case we'll enter
anyway, we want the map to be declared in the whole function to be used below.

Seems like you should add anelse block which populatesdisplayedTags if the provision key is nil?

I don't think what you have is a big deal performance-wise since maps can never shrink, so if themake call on L107 has a length of 4 and the call on L118 has a length of 2, it'll keep the originally-allocated memory.
Preallocation is not strictly necessary but it's a good practice. In this case if there's a chance we'll allocate twice then there's not much of a saving since that's what preallocation is trying to prevent.

tried to improve it from your feedback, was it what you had in mind ? 🙏

I basically allocate before the logic, then fill it with the tags i want based on the case - worst situation, it will reallocate with more space for the tags fetched if required.

I'm not really understanding the point ofNoProvisionerKeyFound. The purpose of this PR is to add changes to tags which are logged, but AFAICS you're not looking at the outputted logs? Does the provisioner fail to start in this case?

Seems like you should add anelse block which populatesdisplayedTags if the provision key is nil?

I don't think what you have is a big deal performance-wise since maps can never shrink, so if themake call on L107 has a length of 4 and the call on L118 has a length of 2, it'll keep the originally-allocated memory.
Preallocation is not strictly necessary but it's a good practice. In this case if there's a chance we'll allocate twice then there's not much of a saving since that's what preallocation is trying to prevent.