remove the check for values begining with numbers will fix #337

[ramonskie]

values are allowed to start with numbers see: https://cloud.google.com/resource-manager/docs/tags/tags-creating-and-managing#adding_tag_values

[rkoster]

This would be a breaking change. We would need an option to opt-out of this change.

[max-soe]

Hi @rkoster, I understand that it's meanwhile there for a long time, but why it's a breaking change? If you define a value that begins with a number you get now a different tag as expected without any notice. BOSH not shows that it changing the label. It's doing this in the background. BOSH also add this to the custom_metadata and there it's using the correct value. So we have also different values for one instance. So I also think that a lot of user not aware that bosh change their value here.

And if we really need an option, how should we phrase this? The behaviour is that it prefix a n to values that start with an integer. If someone really wants to have this n, he can add this to his tags configuration.

[rkoster]

It is a breaking change because it has been this way for a long time and people have build a lot of automation around using tags for chargeback. In doing so they probably have worked around the n prefix. So getting rid of it will be a breaking change.

[max-soe]

I understood but there is already a solution for it. Change the value in your manifest and add the prefix there. So what should we change here? I agree that it make sense to add this to the release notes, but what else do you expect from this PR?

[beyhan]

@rkoster ping ^

[klakin-pivotal]

Hey, @ramonskie and others.

This PR causes our integration tests to fail. We have reverted the changes introduced by this PR. The code you modified is used to check the name of both Tags and Labels. If you want to relax requirements for Tag names, then you'll need to submit a new PR that creates separate code that handles Tag names, and leaves the code that handles Labels as is.

The details of the integration test failures are in the commit message for the revert commit: https://github.com/cloudfoundry/bosh-google-cpi-release/commit/729f446ff375f9d750261c8494b493ad269ca589.

For more information about the restrictions for Label names, see the attached screenshot at the bottom of this message.

Additionally, we're concerned about what appears to be a sentiment that this is a breaking change and users can resolve the breakage by modifying their manifests and redeploying.

Was there some communication not documented in this PR that happened between the time that this comment was added https://github.com/cloudfoundry/bosh-google-cpi-release/pull/338#issuecomment-1483034820 and the time that @rkoster approved this PR that came up with some other workaround for affected users, or perhaps revealed that this concern was not a real one? If there is, would you folks be so kind as to share the conversation with us?

[beyhan]

Hi @klakin-pivotal,

thank you for the detailed summary and for taking care for the integration tests! We should definitely refactor the code if we relax the requirements for the tag names because it is misleading at the moment talking about labels but handling also tags. Regarding the label requirements here are the GCP docs, which describe what is shown on the screenshot above.

Additionally, we're concerned about what appears to be a sentiment that this is a breaking change and users can resolve >the breakage by modifying their manifests and redeploying.

The communication was during the FI WG meeting yesterday on 13th of April. You can check the recording here from 5:30 on. The decision was that it is much easier to apply the manifest changes if needed compared to implementing an opt-in behaviour in bosh and we shouldn't manipulate tag/label if they are allowed on IaaS provider side. I agree that we didn't make good work to document the decision here.

[rkoster]

@klakin-pivotal there also has been some discussion with @mrosecrance about this topic. If we want to keep the old behavior we should do so in OpsMan. From an OSS perspective this change can be seen as a bug fix.

[klakin-pivotal]

The communication was during the FI WG meeting yesterday on 13th of April. You can check the recording here from 5:30 on.

@beyhan Are there written minutes/summaries published from these meetings, or does the CFF generally only publish video recordings of meetings?

[beyhan]

@klakin-pivotal we have meeting notes. We add stuff to the meeting notes which is not related to any issue or pr. Usually, we go over the bosh project board and discuss the issues/prs there. We document our discussions in the prs/issues. The TOC meeting has also detailed meeting notes you can find it in the community calendar. I don't know how other WG are handling this. If you have any suggestion what could be improved in the FI WG meeting please join the meeting to discuss those.