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

Closes #1425 - Migrate Release Pipeline to GitHub Actions #1516

Merged
merged 17 commits into from
Feb 10, 2023

Conversation

aaronweissler
Copy link
Member

@aaronweissler aaronweissler commented Sep 1, 2022

Closes #1425


This change is Reviewable

Copy link
Member Author

@aaronweissler aaronweissler left a comment

Choose a reason for hiding this comment

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

Depends on new release environment with Docker login data and a protection rule so releases are only created with explicit approval.

Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions


.github/workflows/release.yml line 26 at r1 (raw file):

    runs-on: ubuntu-latest
    environment: release
    needs: [agent_test, configdocsgenerator_test, configurationserver_test, configuration_ui_test]

Open question:
Do we want to run all tests once more when triggering a release?
(this is what would happen with the current version)

I personally feel like it is not necessary, since the tests are already run on every commit to the master anyways, and then they just take up time for the release, especially if one of them fails randomly, like some of them do at times right now.


components/inspectit-ocelot-configurationserver/build.gradle line 168 at r1 (raw file):

    dependsOn copyServerJar

    tag 'versioned', "hub.docker.com/${name}:${version}"

tags is deprecated in favor of tag

@aaronweissler
Copy link
Member Author

When merging, CircleCI must be deactivated in the project's settings.

@aaronweissler
Copy link
Member Author

Currently Releases would be shown as created by github-actions user, if we want it to be by a specific user, e.g. NTTechnicalUser I think we need to just provide a personal access token of his to the release action, but the docs are not entirely clear on that, so we should test that once if wanted.

@aaronweissler aaronweissler marked this pull request as draft September 12, 2022 08:32
@aaronweissler
Copy link
Member Author

Draft until NTTechnicalUser as releasing user

@aaronweissler aaronweissler marked this pull request as ready for review September 12, 2022 09:48
@aaronweissler
Copy link
Member Author

Token has been added to code and environment secrets and functionality has been tested in fork.

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 10 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @aaronweissler and @Dimi-Ma)


.github/workflows/agent_test.yml line 19 at r3 (raw file):

      - '.github/**'
      - '.circleci/**'
  workflow_call:

do we need this workflow_call (also in the other files) so that release.yml can use it?


.github/workflows/deploy_master_docs.yml line 1 at r3 (raw file):

name: Deploy Master Documentation

this does not release a versioned documentation, right?


.github/workflows/deploy_master_docs.yml line 22 at r3 (raw file):

        uses: actions/checkout@v3
      - name: Set up Node.js
        uses: actions/setup-node@v2

in the docs_deployment_test.yml, we are using actions/setup-node@v3


.github/workflows/docs_deployment_test.yml line 16 at r3 (raw file):

        working-directory: inspectit-ocelot-documentation/website
    steps:
      - uses: actions/checkout@v2

in the deploy_master_docs.yml, we are using actions/checkout@v3


.github/workflows/release.yml line 89 at r3 (raw file):

        uses: actions/checkout@v3
      - name: Setup Node.js
        uses: actions/setup-node@v2

can we use actions/setup-node@v3?

@heiko-holz heiko-holz self-assigned this Nov 21, 2022
Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @aaronweissler and @Dimi-Ma)


.github/workflows/release.yml line 73 at r3 (raw file):

        run: |
          docker login -u ${{ secrets.DOCKER_HUB_USER }} -p  ${{ secrets.DOCKER_HUB_PASSWORD }}
          docker push aaronweikru/inspectit-ocelot-agent:${{ github.ref_name }}

before accepting the pull request, we need to change this to the inspectIT Ocelot main repository


.github/workflows/release.yml line 89 at r3 (raw file):

setup-node@v3?

Copy link
Contributor

@danipaniii danipaniii left a comment

Choose a reason for hiding this comment

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

Unfortunately I cannot push to Aarons branch, so I made a branch on my fork and implemented the above mentioned changes.

Reviewable status: 8 of 11 files reviewed, 8 unresolved discussions (waiting on @aaronweissler, @Dimi-Ma, and @heiko-holz)


.github/workflows/agent_test.yml line 19 at r3 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

do we need this workflow_call (also in the other files) so that release.yml can use it?

Yes.


.github/workflows/deploy_master_docs.yml line 1 at r3 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

this does not release a versioned documentation, right?

I will try out.


.github/workflows/deploy_master_docs.yml line 22 at r3 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

in the docs_deployment_test.yml, we are using actions/setup-node@v3

Indeed, it probably won't affect the result for now, but it is better to keep things consistent, so I will change it.


.github/workflows/docs_deployment_test.yml line 16 at r3 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

in the deploy_master_docs.yml, we are using actions/checkout@v3

I also updated it here, to stay consistent.


.github/workflows/release.yml line 26 at r1 (raw file):

Previously, aaronweissler wrote…

Open question:
Do we want to run all tests once more when triggering a release?

I personally feel like it is not necessary, since the tests are already run on every commit to the master anyways, and then they just take up time for the release, especially if one of them fails randomly, like some of them do at times right now.

I agree with Aaron, that running the tests when triggering a release is not really necessary.


.github/workflows/release.yml line 73 at r3 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

before accepting the pull request, we need to change this to the inspectIT Ocelot main repository

Are the secrets already set to login into Dockerhub?


.github/workflows/release.yml line 89 at r3 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

setup-node@v3?

Yes, I changed it.

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aaronweissler, @danipaniii, and @Dimi-Ma)


.github/workflows/agent_test.yml line 19 at r3 (raw file):

Previously, danipaniii wrote…

Yes.

thx


.github/workflows/release.yml line 73 at r3 (raw file):

Previously, danipaniii wrote…

Are the secrets already set to login into Dockerhub?

Good question, I don't know as I've never worked with the Dockerhub :(

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aaronweissler, @danipaniii, and @Dimi-Ma)


.github/workflows/release.yml line 26 at r1 (raw file):

Previously, aaronweissler wrote…

Open question:
Do we want to run all tests once more when triggering a release?

I personally feel like it is not necessary, since the tests are already run on every commit to the master anyways, and then they just take up time for the release, especially if one of them fails randomly, like some of them do at times right now.

I would agree.
But only if we cannot start a release before the tests on the main branch have passed (as they are triggered after the commit). If we can also start the release without waiting for the tests after the last commit, I would still have the tests in the release ...

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @aaronweissler, @danipaniii, and @Dimi-Ma)

a discussion (no related file):
As far as I can see you were able to push to Aaron's branch, right?


Copy link
Member Author

@aaronweissler aaronweissler left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Dimi-Ma and @heiko-holz)


.github/workflows/deploy_master_docs.yml line 1 at r3 (raw file):

Previously, danipaniii wrote…

I will try out.

Just in case this hasn't been answered yet, yes, this action is only for updating the master branch version of the documentation. The versioned documentation is generated only for releases in the build_documentation step of the release action.


.github/workflows/release.yml line 73 at r3 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Good question, I don't know as I've never worked with the Dockerhub :(

Unless the login data changed since then, yes they are set up. You can see the secrets (DOCKER_HUB_USER and DOCKER_HUB_PASSWORD) in the release environment in the project settings.

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aaronweissler and @Dimi-Ma)


.github/workflows/deploy_master_docs.yml line 1 at r3 (raw file):

Previously, aaronweissler wrote…

Just in case this hasn't been answered yet, yes, this action is only for updating the master branch version of the documentation. The versioned documentation is generated only for releases in the build_documentation step of the release action.

Thanks @aaronweissler for your clarification!

@danipaniii
Copy link
Contributor

When trying to make a test-release I encounter two errors:

  1. Pushing the docker image results in the error message Cannot perform an interactive login from a non TTY device. Which may result because of the wrong credentials. Another answer from the internet suggested that it is because the $DOCKER_TAGenvironmental isn't set. But trying to set the variable did not fix the problem.
  2. When creating a new documentation results in the error message pathspec 'master' did not match any file(s) known to git

At the moment I have actions running, trying to fix the above mentioned error messages.
For more information on how to maybe fixing the 2. issue I found the following stack overflow post: https://stackoverflow.com/questions/5989592/git-cannot-checkout-branch-error-pathspec-did-not-match-any-files-kn.

@danipaniii
Copy link
Contributor

danipaniii commented Jan 23, 2023

I could fix the issue with creating the documentation and I started to implement the docker github action from the ocelot eum server, because I run into more errors with the login with the previous action.
Unfortunately when building the docker image I get the following error:
Screenshot from 2023-01-23 12-34-53

Extra info:

  • The entrypoint.sh is in the same directory as the Dockerfile
  • The inspectit jar is in the same directory and gets added without problem
  • Using COPY or ADD did not result in a different result

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 13 files reviewed, 4 unresolved discussions (waiting on @aaronweissler and @Dimi-Ma)


.github/release.yml line 12 at r6 (raw file):
To be consistent with previous releases, let's change

New Features 🎉

to

Implemented Enhancements 🎉

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r7, all commit messages.
Reviewable status: 10 of 14 files reviewed, 8 unresolved discussions (waiting on @aaronweissler and @Dimi-Ma)


.github/workflows/changelog-release-config.json line 4 at r7 (raw file):

    "categories": [
        {
            "title": "## 🛠 Breaking Changs",

missing "e" in "Changes"


.github/workflows/changelog-release-config.json line 24 at r7 (raw file):

        }
    ],
    "sort": "ASC",

what are the sorting options?
Previously, we have sorted by the closing date


.github/workflows/release.yml line 55 at r4 (raw file):

          token: ${{ secrets.RELEASE_USER_TOKEN }}
          name: Version ${{ github.ref_name }}
          body: "You can also find the corresponding documentation online under the following link: [inspectIT Ocelot Documentation](http://docs.inspectit.rocks)"

is this sentence also present in the new changelog?


.github/workflows/release.yml line 81 at r7 (raw file):

      - name: Checkout
        uses: actions/checkout@v3
      # Downloading jars to create docker image

Why are the longer steps needed to publish to docker?

What didn't work with Aaron's original approach (which is way shorter).

Aaron was using the docker tasks in the agent's build.gradle file.

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 14 files reviewed, 7 unresolved discussions (waiting on @aaronweissler and @Dimi-Ma)


.github/workflows/release.yml line 81 at r7 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Why are the longer steps needed to publish to docker?

What didn't work with Aaron's original approach (which is way shorter).

Aaron was using the docker tasks in the agent's build.gradle file.

let's discuss this in our next meeting

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 14 files reviewed, 9 unresolved discussions (waiting on @aaronweissler and @Dimi-Ma)


components/inspectit-ocelot-configurationserver/docker/Dockerfile line 3 at r7 (raw file):

FROM openjdk:11-jre-slim

ADD ./components/inspectit-ocelot-configurationserver/docker/  /

I have found the problem why you couldn't run the gradle task: the current directory is already the /components/inspectit-ocelot-configurationserver/docker when executing the gradle task and the Dockerfile,

thus, this line needs to be changed to
./ /
if we are using the gradlew docker tasks

Code quote:

COPY

inspectit-ocelot-agent/docker/Dockerfile line 3 at r7 (raw file):

FROM busybox

ADD ./inspectit-ocelot-agent/docker/  /

I have found the problem why you couldn't run the gradle task: the current directory is already the /inspectit-ocelot-agent/docker when executing the gradle task and the Dockerfile,

thus, this line needs to be changed to
./ /
if we are using the gradlew docker tasks

Copy link
Contributor

@danipaniii danipaniii left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 14 files reviewed, 9 unresolved discussions (waiting on @aaronweissler, @Dimi-Ma, and @heiko-holz)


.github/workflows/changelog-release-config.json line 4 at r7 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

missing "e" in "Changes"

Done


.github/workflows/changelog-release-config.json line 24 at r7 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

what are the sorting options?
Previously, we have sorted by the closing date

You can sort on ascending or descending order and based either on 'mergedAt' or by 'title'. The default is mergedAt.


.github/workflows/release.yml line 55 at r4 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

is this sentence also present in the new changelog?

It wasn't so I added it now.


.github/workflows/release.yml line 81 at r7 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

let's discuss this in our next meeting

There were the errors with the login and the Dockerfile couldn't find the necessary files to create the image.
The reason for why the login failed, was because no login credentials where set, unfortunately the error message on that was a little misleading.
The solution for the other error you have figured out.


components/inspectit-ocelot-configurationserver/docker/Dockerfile line 3 at r7 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

I have found the problem why you couldn't run the gradle task: the current directory is already the /components/inspectit-ocelot-configurationserver/docker when executing the gradle task and the Dockerfile,

thus, this line needs to be changed to
./ /
if we are using the gradlew docker tasks

Yes, this fixed the problem!

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r6, 2 of 2 files at r8, 3 of 3 files at r9.
Reviewable status: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @aaronweissler and @Dimi-Ma)


.github/workflows/changelog-release-config.json line 24 at r7 (raw file):

Previously, danipaniii wrote…

You can sort on ascending or descending order and based either on 'mergedAt' or by 'title'. The default is mergedAt.

Alright, thanks for clarifying!

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r7, all commit messages.
Dismissed @aaronweissler from a discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aaronweissler and @Dimi-Ma)


components/inspectit-ocelot-configurationserver/build.gradle line 168 at r1 (raw file):

Previously, aaronweissler wrote…

tags is deprecated in favor of tag

ack

@heiko-holz heiko-holz force-pushed the feature/github-actions branch from 7f85399 to 9e501eb Compare February 10, 2023 13:21
Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 11 files at r10, 7 of 7 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aaronweissler and @Dimi-Ma)

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aaronweissler and @Dimi-Ma)

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Dismissed @aaronweissler from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Dimi-Ma)

@heiko-holz heiko-holz merged commit b2dd6d6 into inspectIT:master Feb 10, 2023
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.

Migrate CircleCI to GitHub Actions
4 participants