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

NR-329534 Automate release of nri-jmx #158

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

sairaj18
Copy link
Contributor

Added the following files:

  1. automated_release.yaml - uses reusable_release_automation.yaml file of coreint-automation
  2. security.yaml - uses reusable_security.yaml file of coreint-automation

Updated the following files:

  1. prerelease.yml to use reusable_pre_release.yaml
  2. push_pr.yml to use reusable_push_pr.yaml
  3. release.yml to use reusable_on_release.yaml
  4. repolinter.yml to use reusable_repolinter.yaml

@sairaj18 sairaj18 requested a review from a team as a code owner November 21, 2024 12:46
Copy link

@sigilioso sigilioso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this. Find some comments below:

windows_goarch_matrix: ["amd64"] # 386 not supported in jmx integrations
win_package_type: exe # JMX integrations are shipped in .exe format
publish_schema: "ohi-jmx" # ohi-jmx for integrations that bundle JMX on windows installers
NRJMX_VERSION: '2.6.0' # TODO check if we should update it to latest release version

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.6.0 is the latest, isn't it? Would it make sense having a custom rule to make renovate updating this dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see v3.7.1 as the latest version in the releases tab

Copy link

@sigilioso sigilioso Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That link is for this repo, nri-jmx, but it is pointing to nrjmx, isn't it? https://github.com/newrelic/nrjmx/releases (these similar names are confusing! 😅)

go-version-file: 'go.mod'
# reusable_push_pr contains static-analysis but it does not contain the Semgrep step
# static-analysis job in reusable_push_pr cannot be disabled
# uncommenting the below leads to the following error "creating validator: parsing markdown: parsing markdown headers: unexpected additional L1 header \"2.4.7 (2021-06-10)\" found, only a single L1 header is allowed"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the error due to the changelog format or is it something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it was due to the changelog format. Now we are not getting that error I made changes to the changelog.md file to fix the parsing error

Comment on lines +17 to +27
# TODO check if we can remove the below code and ignore the Semgrep step
# static-analysis:
# name: Run all static analysis checks
# runs-on: ubuntu-20.04
# steps:
# - uses: actions/checkout@v2
# - uses: newrelic/newrelic-infra-checkers@v1
# - name: Semgrep
# uses: returntocorp/semgrep-action@v1
# with:
# auditOn: push

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was removed from other repositories. Ex nri-postgresql. I'd double-check with the team if it is required here for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it was removed in nri-postgresql then it makes sense to remove the static-analysis step as the remaining part is present in reusable workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @paologallinaharbur can you confirm if we can ignore the Semgrep step in the static-analysis job, similar to the above mentioned nri-postgresql

Comment on lines +10 to +11
### enhancements
- leveraged reusable workflows for pipelines

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't include the pipeline changes in the changelog (after all they shouldn't change the integration at all). I don't know if we could take the chance to update the golang version and include that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I be changing from 1.22.9 -> 1.23.2 ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if we are including some changes related to the code itself (dependency bumps, bug-fixes, features, ...). I'd say that changes such as pipelines or dev-dependencies (Eg: a testify upgrade) should not be included in the changelog and should not generate a new release

uses: actions/setup-go@v5
with:
go-version-file: 'go.mod'
# reusable_push_pr contains static-analysis but it does not contain the Semgrep step

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we managed to use the reusable static-analysis we we could get rid of the tools module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep we can remove the commented static-analysis job as it is managed by reusable workflow.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd keep newlines at the end of files. (See this rationale)

@sigilioso sigilioso requested a review from a team November 22, 2024 07:23
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.

2 participants