NH-55892 upload workflow #1
Conversation
There was a problem hiding this comment.
Thanks @xuan-cao-swi! I left some comments, and similar to the PR for apm-ruby let's use the automatic GITHUB_TOKEN for this workflow too.
Another question is: do you intend to merge our changes directly to main, or create a long-lived "solarwinds" branch that would hold our custom code? The latter would require more branch management but allow us better control on what upstream pieces to merge in, visibility and code review of our changes (for example add a CODEOWNERS file for our branch).
| description: 'The path that package you want to publish (should include /)' | ||
| # metrics_sdk/ | ||
| # metrics_api/ | ||
| # exporter/otlp/ |
There was a problem hiding this comment.
Instead of needing an input, we should be able to determine the path to the package based on the gem we want to publish, right? Taking a look at how upstream OTel Ruby does releases, it uses a toys gem which may not suit our needs since it seems to only support releasing to rubygems, but it does have a defined mapping of gems and paths which IMHO is a good idea.
There was a problem hiding this comment.
@xuan-cao-swi have you tried the defined mapping approach that @cheempz suggested here?
IIUC, the sw_build_and_push_gem.sh script finds the correct path given GEM_NAME and now exits with 1 if it cannot be found. I'm wondering if a defined mapping could save on errors from typos.
There was a problem hiding this comment.
Yes, I looked into the toys library but unfortunately, if I want to use toy, then I have to make modification on upstream files, which may cause file conflicts in the future when I pull from upstream. That's why I choose to explore the shell script approach. Plus, the pre-defined mapping only works for existing GEM_NAME, if I want to introduce a new gem, then in that case I have to change the mapping again.
| description: 'The name that package you want to publish' | ||
| # opentelemetry-metrics-sdk | ||
| # opentelemetry-metrics-api | ||
| # opentelemetry-exporter-otlp |
There was a problem hiding this comment.
Can you put in an actual gem name from our fork that we'd publish in the suggestion above, and remove these comments which don't apply to this repo?
| echo -e "---\n:github: Bearer $GITHUB_SECRET_TOKEN" >> ~/.gem/credentials | ||
| chmod 0600 ~/.gem/credentials | ||
| env: | ||
| GITHUB_SECRET_TOKEN: ${{ secrets.GH_PACKAGE_TOKEN }} |
There was a problem hiding this comment.
Let's change this to use GITHUB_TOKEN.
Co-authored-by: Lin Lin <lin.lin@solarwinds.com>
Co-authored-by: Lin Lin <lin.lin@solarwinds.com>
Yes, I intend to merge to main. |
| - name: Setup secrets | ||
| run: | | ||
| mkdir ~/.gem | ||
| echo -e "---\n:github: Bearer $GITHUB_SECRET_TOKEN" >> ~/.gem/credentials |
There was a problem hiding this comment.
I think this file could be shortened by not setting env below and directly using secrets.GITHUB_TOKEN
There was a problem hiding this comment.
I don't think that will work. Do you have any reference that I can take look?
There was a problem hiding this comment.
Ah ok. We can keep it that way.
|
|
||
| GEM_NAME="$1" | ||
|
|
||
| cd "instrumentation/$GEM_NAME" |
There was a problem hiding this comment.
I recommend running this script through shellcheck to lint the bash.
There was a problem hiding this comment.
updated with lint for shell
| @@ -0,0 +1,37 @@ | |||
| # Copyright (c) 2023 SolarWinds, LLC. | |||
There was a problem hiding this comment.
Is there a reason for having this header instead of the full Apache license? Same for the other file.
There was a problem hiding this comment.
No particular reason. Just realized there is no header for all workflows so I removed it. Other files are out of my hand since they are from upstream.
| gem_version=$(grep -E "VERSION\s*=\s*'[^']+'" "$found_file" | awk -F "'" '{print $2}') | ||
|
|
||
| # build and push gem | ||
| gem build "opentelemetry-instrumentation-$GEM_NAME.gemspec" |
There was a problem hiding this comment.
What happens with gem build if there is a typo in the input gem name?
There was a problem hiding this comment.
actually, if there is a typo of the gem name, then it won't find the correct path, and based on the linted line 5 (cd "instrumentation/$GEM_NAME" || exit), it will exit from shell script (kind remind me to give an exit status code)
No description provided.