-
Notifications
You must be signed in to change notification settings - Fork 21
feat: create files for plus distro #427
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
base: feature/nrdot-plus
Are you sure you want to change the base?
Conversation
| | Status | | | ||
| |-----------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | Distro | `nrdot-collector-plus` | | ||
| | Stability | `alpha` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "preview" different than "alpha", or are they essentially synonymous in this context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preview in this context refers to 'limited preview' in the sense of a pre-release product.
alpha is the value we chose for something that isn't yet in limited preview, e.g. the core distro.
So in the case of plus, alpha is the correct value.
| distribution: ${{ fromJson(needs.setup.outputs.distributions) }} | ||
| fips: [false, true] | ||
| exclude: | ||
| - fips: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't dug into the syntax but wouldn't this leave plus+non-fips in the matrix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're 100% right, I forgot to commit these changes!
| } | ||
|
|
||
| if baseDist == K8sDistro || fips { | ||
| if baseDist == K8sDistro || baseDist == PlusDistro || fips { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that we'll want binaries for plus just cause it's going to have more overlap with host than any other distro. So probably remove the exclusion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking this back. You're right to skip this for now as the packages don't really make sense without a default config.
I created https://new-relic.atlassian.net/browse/NR-490009 to keep track of this.
| if baseDist == K8sDistro || baseDist == PlusDistro || fips { | ||
| dist.Goos = []string{"linux"} | ||
| dist.IgnoredBuilds = nil | ||
| dist.SkipBinaries = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat unrelated but this block should probably contain SkipArchive=true instead of having that separated out just for fips.
The only difference is that k8s+non-fips would create archives but we don't really want/need that anyway because the helm chart only needs the image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I can add that to this PR, it's a quick fix
| dist: | ||
| module: github.com/newrelic/nrdot-collector-releases/nrdot-collector-plus | ||
| name: nrdot-collector-plus | ||
| description: NRDOT+ Collector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| description: NRDOT+ Collector | |
| description: NRDOT Collector Plus |
I would keep the pattern established by the other distros in case this shows up in logs/attributes and we want to parse the distro for dashboards.
| # Systemd environment file for the nrdot-collector-plus service | ||
| # Command-line options for the nrdot-collector-plus service. | ||
| # See https://opentelemetry.io/docs/collector/configuration/ to see all available options. | ||
| OTELCOL_OPTIONS="--config=/etc/nrdot-collector-plus/config.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we skip package generation, then lets get rid of this for now, same for the service file and shell scripts
Co-authored-by: kb-newrelic <[email protected]>
Creates files for nrdot-collector-plus distribution:
Notes: