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

Add a tool that allows us to quickly update specification URLs to a s… #9

Merged

Conversation

jsuereth
Copy link
Contributor

…pecific version.

This should make it a lot easier for us to direct link against the specification and rapidly update links/check them over time.

@Oberon00
Copy link
Member

Oberon00 commented May 11, 2023

Note that there is an issue for that at the semantic convention generator: open-telemetry/build-tools#20 But since this is now needed also outside the generated markdown, a separate tool might be better anyways. Do consider adding it to the build-tools repository though (or should the semconv generator move here?)

(As long as it's only a short shell script like this, it doesn't really matter though)

@jsuereth
Copy link
Contributor Author

@Oberon00 very good questions. I like to start small and evolve over time, so I think since the script is short and directly tied to semconv maintenance, we can leave it here.

If we discover limitations that necessitate moving it into build-tools I'm more than happy to move the tool over there.

Regarding the evolution of build-tools and this repository, it COULD be we merge them together over time. Let's see how "momentum" goes as we migrate to this new repository and if we find huge friction keeping it separate then we can push for merging.

It's a great call out, let's have everyone keep the idea on our radar.

@jsuereth jsuereth marked this pull request as ready for review May 11, 2023 18:04
@jsuereth jsuereth requested review from a team May 11, 2023 18:04
internal/tools/update_specification_version.sh Outdated Show resolved Hide resolved
internal/tools/update_specification_version.sh Outdated Show resolved Hide resolved
Thanks for the scripting help!

Co-authored-by: Liudmila Molkova <[email protected]>
Co-authored-by: Johannes Tax <[email protected]>
@Oberon00
Copy link
Member

I like the find solution, however, I wonder if we should add a check against adding files with spaces to the repository instead of trying to support spaces here. You probably don't want paths with spaces anyways, and somewhere you most likely will run into a bug anyways.

internal/tools/update_specification_version.sh Outdated Show resolved Hide resolved
Improvements from code review

Co-authored-by: Christian Neumüller <[email protected]>
@jsuereth
Copy link
Contributor Author

Thanks for the improvements!

Regarding spaces in files, remember that's common in windows that your home directory may even have spaces. When using WSL / cygwin, it's really important to handle regardless.

@jsuereth jsuereth requested a review from a team May 16, 2023 16:22
@jsuereth jsuereth merged commit b5bcbcc into open-telemetry:main May 16, 2023
@jsuereth jsuereth deleted the pr-update-spec-version-url-script branch May 16, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants