Skip to content

[fix][CI] Check Pulsar SQL / Trino license files - #15253

Closed
lhotari wants to merge 2 commits into
apache:masterfrom
lhotari:lh-check-presto-license-files
Closed

[fix][CI] Check Pulsar SQL / Trino license files#15253
lhotari wants to merge 2 commits into
apache:masterfrom
lhotari:lh-check-presto-license-files

Conversation

@lhotari

@lhotari lhotari commented Apr 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes #16783

Motivation

The new "Pulsar CI" workflow doesn't currently check Pulsar SQL (Trino) license files.
The existing "build-and-license-check" builds only the core-modules profile because of performance reasons.

Modifications

Add license check to the step that builds the full apachepulsar/pulsar-test-latest-version:latest docker image.

@lhotari lhotari added doc-not-needed Your PR changes do not impact docs area/ci labels Apr 21, 2022
@lhotari
lhotari requested review from eolivelli and gaoran10 April 21, 2022 11:34
@lhotari lhotari self-assigned this Apr 21, 2022
@lhotari
lhotari force-pushed the lh-check-presto-license-files branch from 79904d6 to b981a91 Compare April 22, 2022 08:29
@github-actions

github-actions Bot commented May 23, 2022

Copy link
Copy Markdown

The pr had no activity for 30 days, mark with Stale label.

@github-actions

github-actions Bot commented Jun 22, 2022

Copy link
Copy Markdown

The pr had no activity for 30 days, mark with Stale label.

@lhotari lhotari mentioned this pull request Sep 15, 2022
Closed
@lhotari
lhotari force-pushed the lh-check-presto-license-files branch from b981a91 to cd0b096 Compare September 15, 2022 06:31
@lhotari lhotari changed the title [CI] Check Presto license files [CI] Check Trino license files Sep 15, 2022
@lhotari lhotari changed the title [CI] Check Trino license files [fix][CI] Check Pulsar SQL / Trino license files Sep 15, 2022
@lhotari

lhotari commented Sep 15, 2022

Copy link
Copy Markdown
Member Author

@tisonkun Please review

@lhotari lhotari mentioned this pull request Sep 15, 2022
Merged
@github-actions github-actions Bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Sep 15, 2022
@github-actions

github-actions Bot commented Sep 15, 2022

Copy link
Copy Markdown

@lhotari Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

nicoloboschi
nicoloboschi approved these changes Sep 15, 2022
View reviewed changes

@nicoloboschi nicoloboschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if you did it on purpose but running this check also in the Build and License check step is a good thing because it can avoid unnecessary steps to be executed in case of failure

tisonkun
tisonkun reviewed Sep 15, 2022
View reviewed changes

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we remove these lines:

pulsar/.github/workflows/pulsar-ci.yaml

Lines 106 to 107 in cd0b096

- name: Check binary licenses
run: src/check-binary-license.sh ./distribution/server/target/apache-pulsar-*-bin.tar.gz

since it's duplicated by the new lines. Then we may save some cycles.

@github-actions github-actions Bot removed the Stale label Sep 16, 2022
@lhotari

lhotari commented Sep 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

I don't know if you did it on purpose but running this check also in the Build and License check step is a good thing because it can avoid unnecessary steps to be executed in case of failure

@nicoloboschi @tisonkun It's intentionally not handled in "Build and License check". if it would be done there, we'd have to compile the whole distribution and that would mess up the whole "Build and License check" step.

tisonkun
tisonkun approved these changes Sep 20, 2022
View reviewed changes

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@lhotari I know that we don't have to compile the whole distribution in the first place. My point is that since we check the whole binary in this new step, we can remove the former step. But the former step can help fail fast, so it's somewhat reasonable to keep as is.

@lhotari
lhotari force-pushed the lh-check-presto-license-files branch from cd0b096 to 6b2d578 Compare September 27, 2022 04:30
@tisonkun

tisonkun commented Oct 17, 2022

Copy link
Copy Markdown
Member

@lhotari any progress here?

tisonkun added a commit to tisonkun/pulsar that referenced this pull request Oct 24, 2022
@tisonkun
[fix][ci] Check full build artifacts licenses
27fd9b9 
This closes apache#16783.
This closes apache#15253.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun mentioned this pull request Oct 24, 2022
Merged
4 tasks
@tisonkun

tisonkun commented Oct 24, 2022

Copy link
Copy Markdown
Member

No responses. Try to supersede in #18171.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tisonkun tisonkun tisonkun approved these changes
@nicoloboschi nicoloboschi nicoloboschi approved these changes
@eolivelli eolivelli Awaiting requested review from eolivelli
@gaoran10 gaoran10 Awaiting requested review from gaoran10

Assignees

@lhotari lhotari

Labels

area/ci doc-label-missing lifecycle/stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

check-binary-license.sh doesn't check Pulsar SQL licenses in CI

3 participants

@lhotari @tisonkun @nicoloboschi