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.

Sign up for GitHub

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

Jump to bottom

Task #905 - Add scan-licenses.yml GA workflow #145

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

Task #905 - Add scan-licenses.yml GA workflow #145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a7c08d8
Add scan-licenses.yml GA workflow
anamarijabalaban Oct 11, 2023
e5a8211
Update scan-licenses.yml workflow
anamarijabalaban Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/workflows/scan-licenses.yml
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

How are the developers notified that there are issues with some licenses?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, we should setup a Slack notification here.

DevOps used https://github.com/rtCamp/action-slack-notify

Copy link
Member

Choose a reason for hiding this comment

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

I don't think Slack notification is necessary for this step.

Copy link
Author

Choose a reason for hiding this comment

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

@nikajukic I tried to use https://github.com/rtCamp/action-slack-notify for sending slack notifications but it looks like they require us to install this https://slack.com/apps/A0F7XDUAZ-incoming-webhooks?tab=settings&next_id=0

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
name: Scan licenses

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

on:
workflow_call:

jobs:
scanning:
runs-on: ubuntu-latest
name: License scanning
steps:
- name: Git checkout
uses: actions/checkout@v3
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
bundler-cache: true
- name: Run license scanning
run: |
bundle config --local deployment false
bundle lock --add-platform x86_64-linux
bundle config --local deployment true
Comment on lines +23 to +25
Copy link
Member

Choose a reason for hiding this comment

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

This block looks like it's trying to perform a trick/hack. Why do we need this? Is there a more conventional way of achieving the same result?

Copy link
Author

Choose a reason for hiding this comment

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

First, I tried just to call bundle exec license_finder but I got some missing nokogiri error #145 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Seems like you uncovered a problem with our setup. This is a native extension compilation problem, which originates from our template.rb specifically here.

Nokogiri installation docs contains compelling arguments to explicitly list platforms making bundler install precompiled binaries instead of compiling deps natively.

@lovro-bikic your comment here was the trigger to move from explicitly listing platforms to using ruby. Can you please take a look at this problem and add your thoughts on the topic?

bundle exec license_finder
Comment on lines +23 to +26
Copy link
Member

Choose a reason for hiding this comment

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

I'd actually like to have this as a script in the project, similar to e.g. bin/lint:

default_rails_template/template.rb

Lines 164 to 178 in ce3d228

BIN_LINT = <<~HEREDOC.strip_heredoc
#!/usr/bin/env bash
set -o errexit
set -o pipefail
set -o nounset
echo "=========== zeitwerk check ==========="
time bundle exec rails zeitwerk:check
echo "=========== rubocop ==========="
time bundle exec rubocop --format simple --format github --color --parallel
HEREDOC
create_file 'bin/lint', BIN_LINT, force: true
chmod 'bin/lint', 0755, verbose: false

That way developers can run the script locally and modify it if needed.

Copy link
Member

Choose a reason for hiding this comment

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

For simple one/two-liners, I find it's more complex when CI workflow is referencing project scripts. GH workflow is coupled with the default_rails_template, and it makes it less portable.

In this case, I'm OK with both approaches, but in general, I'm in favor of workflow being self-sustainable, and project scripts should be used only where necessary.

12 changes: 12 additions & 0 deletions scan-licenses.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
name: Scan licenses

on:
workflow_dispatch:
push:
paths:
- 'Gemfile*'
- 'package.json'

jobs:
scanning:
uses: infinum/default_rails_template/.github/workflows/scan-licenses.yml@v1
1 change: 1 addition & 0 deletions template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,7 @@ def run

get("#{BASE_URL}/build.yml", '.github/workflows/build.yml')
get("#{BASE_URL}/delete-cache.yml", '.github/workflows/delete-cache.yml')
get("#{BASE_URL}/scan-licenses.yml", '.github/workflows/scan-licenses.yml')

## Docker
if no?('Will this application use Docker? [Yes]', :green)
Expand Down