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

common: Update configure runner and opensuse setup scripts #5816

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

szczepax
Copy link
Contributor

@szczepax szczepax commented Jul 21, 2023

  • Update actions-runner release version to v2.306.0,
  • chown should also change the group for actions-runner folder,
  • ensure HOME and PKG_CONFIG_PATH are propagated to actions-runner environment
  • remove stale reference to install-libndctl.sh in opensuse-setup

This change is Reviewable

@szczepax szczepax changed the title Update configure runner and opensuse setup scripts common: Update configure runner and opensuse setup scripts Jul 21, 2023
Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @szczepax)

a discussion (no related file):
You may also want to fix this line in the commit message:

chown should also change the group for actions-runner folder,



utils/ansible/configure-self-hosted-runner.yml line 62 at r1 (raw file):

    - name: "Change owner to {{ testUser }}"
      shell: chown -R $(id -u {{ testUser }}).$(id -g {{ testUser }}) {{ runner_folder }}

I believe you should use : instead of . to separate OWNER and GROUP.

Suggestion:

chown -R $(id -u {{ testUser }}):$(id -g {{ testUser }}) {{ runner_folder }}

utils/ansible/configure-self-hosted-runner.yml line 65 at r1 (raw file):

    # Make sure the following environment variables are present in the env
    # to ensure propagation to the actions-runner environment.

Suggestion:

    # Make sure the following environment variables are present in the env
    # to ensure propagation to the actions-runner's environment.

utils/ansible/configure-self-hosted-runner.yml line 70 at r1 (raw file):

        path: "{{ runner_folder }}/env.sh"
        line: "    'PKG_CONFIG_PATH'\n    'HOME'"
        insertafter: "^varCheckList=\\("

Nothing major but squeezing these two lines into a single one made it harder for me to follow. Please consider using a loop instead.

Suggestion:

    - name: "Add env variables into env.sh checklist"
      lineinfile:
        path: "{{ runner_folder }}/env.sh"
        line: "    '{{ item.line }}'"
        insertafter: "^varCheckList=\\("
      loop:
        - line: PKG_CONFIG_PATH
        - line: HOME

Copy link
Contributor

@janekmi janekmi 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 @szczepax)

a discussion (no related file):
Please prepend the commit message with the common: prefix as well.


Copy link
Contributor

@osalyk osalyk 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 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @szczepax)

@szczepax szczepax requested a review from janekmi July 25, 2023 11:59
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Merging #5816 (38500e7) into master (fafc805) will increase coverage by 0.07%.
Report is 43 commits behind head on master.
The diff coverage is n/a.

❗ Current head 38500e7 differs from pull request most recent head b067ebb. Consider uploading reports for the commit b067ebb to get more accurate results

@@            Coverage Diff             @@
##           master    #5816      +/-   ##
==========================================
+ Coverage   70.93%   71.00%   +0.07%     
==========================================
  Files         131      131              
  Lines       19175    19175              
  Branches     3192     3193       +1     
==========================================
+ Hits        13602    13616      +14     
+ Misses       5573     5559      -14     

Copy link
Contributor

@janekmi janekmi 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 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @szczepax)

a discussion (no related file):
Please squash all commits to a single one.


common: Update configure runner and opensuse setup scripts
Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @szczepax)

@janekmi janekmi added the no changelog Add to skip the changelog check on your pull request label Aug 7, 2023
Copy link
Contributor Author

@szczepax szczepax 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 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @szczepax)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @szczepax)

Copy link
Contributor

@janekmi janekmi 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @szczepax)

@janekmi janekmi merged commit 6e4f445 into pmem:master Aug 23, 2023
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Add to skip the changelog check on your pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants