Skip to content

Add option to not install the project#18

Merged
erikjohnston merged 3 commits into
release/v1from
erikj/install_project
Mar 28, 2023
Merged

Add option to not install the project#18
erikjohnston merged 3 commits into
release/v1from
erikj/install_project

Conversation

@erikjohnston

Copy link
Copy Markdown
Member

Fixes #15

@erikjohnston erikjohnston requested a review from a team as a code owner March 28, 2023 13:34
Comment thread action.yml Outdated
if: inputs.extras != '' || inputs.groups != ''
# (Empty extras or groups lists are fine.)
run: poetry install --no-interaction --extras="${{ inputs.extras }}" --with="${{ inputs.groups }}"
run: poetry install --no-interaction --extras="${{ inputs.extras }}" --with="${{ inputs.groups }}" ${{ inputs.install-project == 'true' && '' || '--no-root' }}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No wait, '' is falsey so this won't work nyargh

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why does GHA not have a ternary syntax :((

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh bugger, I just trusted you here! https://github.com/orgs/community/discussions/26738 has some context

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

GHA would probably say "if it's complicated, write it in javascript" tbh

@DMRobertson DMRobertson Mar 28, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't you want {{ inputs.install-project == 'true' && '--no-root' || '' }}?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

err != 'true'

DMRobertson
DMRobertson previously approved these changes Mar 28, 2023

@DMRobertson DMRobertson left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. It'd be good to test it on a Synapse PR if you could before merging, e.g. by changing it to use setup-python-poetry@erikj/install_project, but I won't block you on doing so.

Comment thread action.yml
Comment on lines 102 to 111
- name: Install project (no extras or groups)
if: inputs.extras == '' && inputs.groups == ''
run: poetry install --no-interaction
run: poetry install --no-interaction ${{ inputs.install-project == 'true' && '' || '--no-root' }}
shell: bash

- name: Install project with --extras=${{ inputs.extras }} --with=${{ inputs.groups }}
if: inputs.extras != '' || inputs.groups != ''
# (Empty extras or groups lists are fine.)
run: poetry install --no-interaction --extras="${{ inputs.extras }}" --with="${{ inputs.groups }}"
run: poetry install --no-interaction --extras="${{ inputs.extras }}" --with="${{ inputs.groups }}" ${{ inputs.install-project == 'true' && '' || '--no-root' }}
shell: bash

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Was a bit confused by why we have these two steps here. I think it's because I couldn't find a cleaner way to only add --extras if inputs.extras was specified: I don't think there's a ternary syntax or helper function?

@DMRobertson DMRobertson dismissed their stale review March 28, 2023 13:45

oops missed the gha nonsesne

@erikjohnston

Copy link
Copy Markdown
Member Author

matrix-org/synapse#15338 seems to now do the right thing, which tests this

@DMRobertson

Copy link
Copy Markdown

matrix-org/synapse#15338 seems to now do the right thing, which tests this

Thanks, I can see that the pydantic job succeeds (so compiled the Rust, ergo installed the project) and that the sampleconfig job failed because it couldn't import Synapse with the new option set. LGTM

@erikjohnston erikjohnston merged commit 73f6764 into release/v1 Mar 28, 2023
@erikjohnston erikjohnston deleted the erikj/install_project branch March 28, 2023 14:05
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.

Option to skip installing the main project

2 participants