-
Notifications
You must be signed in to change notification settings - Fork 37
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 build and test stage #37
Conversation
.github/workflows/build-and-test.yml
Outdated
name: Build and test | ||
on: | ||
pull_request: | ||
branches: [ "main" ] |
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.
why restricted only to main branch?
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 have no explaination for that. Should build be performed for every commit?
.github/workflows/build-and-test.yml
Outdated
run: ansible-galaxy collection install dynatrace-oneagent* | ||
- name: Running sanity test | ||
run: pushd ~/.ansible/collections/ansible_collections/dynatrace/oneagent && ansible-test sanity && popd | ||
- uses: actions/upload-artifact@v4 |
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.
publishing should be done at least as a separate step
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.
What is the reason for that? I don't think putting it to separate job and create dependency between uploading and building just for a sake of separating them has any meaning
.github/workflows/build-and-test.yml
Outdated
with: | ||
python-version: '3.10' | ||
- name: Installing dependencies | ||
run: pip install ansible |
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.
Add step for downloading https://ca.dynatrace.com/dt-root.cert.pem
a373c9d
to
e0c1e20
Compare
@@ -0,0 +1,26 @@ | |||
# Copyright 2023 Dynatrace LLC |
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.
# Copyright 2023 Dynatrace LLC | |
# Copyright 2024 Dynatrace LLC |
build-and-test: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v4 |
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.
Please name the steps
name: Build, test and upload | ||
on: | ||
pull_request: | ||
types: [ 'closed' ] |
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.
why not just main branch? What is the difference?
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.
So that the artifact is only published once the PR is closed
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.
What if PR is closed without merging to main branch? Shouldn't we trigger upload only on main branch?
workflow_dispatch: | ||
|
||
env: | ||
FILES_DIR: roles/oneagent/files |
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.
duplicated in build-and-test.yaml
steps: | ||
- name: Download certificate | ||
shell: bash | ||
run: mkdir $FILES_DIR && wget https://ca.dynatrace.com/dt-root.cert.pem -P $FILES_DIR |
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.
It's worth to add $FILES_DIR
directory creation as a separate step, because it may fail.
run: mkdir $FILES_DIR && wget https://ca.dynatrace.com/dt-root.cert.pem -P $FILES_DIR | ||
- name: Building the collection | ||
shell: bash | ||
run: ansible-galaxy collection build . |
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.
run: ansible-galaxy collection build . | |
run: ansible-galaxy collection build . -vvv |
ansible-galaxy collection build .
does not generally provide much information.
- name: Download certificate | ||
shell: bash | ||
run: mkdir $FILES_DIR && wget https://ca.dynatrace.com/dt-root.cert.pem -P $FILES_DIR | ||
- name: Building the collection |
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.
Please keep to the previously estabilished naming convention.
Building the collection
→ Build the collection
Installing the collection
→ Install the collection
etc.
name: Build and test | ||
on: | ||
pull_request: | ||
branches: [ "*" ] |
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.
When you want to run a given workflow on all branches, you don't need to specify them. Following will work:
on:
pull_request:
push:
workflow_dispatch:
workflow_call:
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.
In such scenario, push will work also on tags
workflow_call: | ||
|
||
env: | ||
FILES_DIR: roles/oneagent/files |
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.
FILES_DIR: roles/oneagent/files | |
CERTIFICATE_DIR: roles/oneagent/files |
name: Build, test and upload | ||
on: | ||
pull_request: | ||
branches: [ 'main' ] |
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.
it should be:
push:
branches:
- main
since we cannot open a PR on main
branch
787b2c8
to
27ed92c
Compare
No description provided.