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

sdk include/exclude logic #21

Merged
merged 8 commits into from
Dec 28, 2023
Merged

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Dec 27, 2023

This PR extends the sdk option to support intuitive include/exclude logic for which packages to publish.

This change is fully backwards compatible.

The motivator is a fix for pulumi/pulumi-local#1 that doesn't require naming all languages to publish.

My hope is that non-pulumi users can also use this to clean up their publish scripts.


This has been tested as part of https://github.com/pulumi/pulumi-local/actions/runs/7339080823/job/19982930394. We can see that java, node and .NET published but python did not.

@iwahbe iwahbe requested review from guineveresaenger and a team December 27, 2023 13:59
@iwahbe iwahbe self-assigned this Dec 27, 2023
iwahbe added a commit to pulumi/pulumi-local that referenced this pull request Dec 27, 2023
iwahbe added a commit to pulumi/pulumi-local that referenced this pull request Dec 27, 2023
action.yml Outdated
run: find "${{ github.workspace }}/sdk/dotnet/bin/Debug/" -name 'Pulumi.*.nupkg'
-exec dotnet nuget push -k "${NUGET_PUBLISH_KEY}" -s https://api.nuget.org/v3/index.json {} \;
shell: bash
- name: Setup Python
if: inputs.sdk == 'python' || inputs.sdk == 'all'
if: (contains(inputs.sdk, 'python') || (contains(inputs.sdk, 'all') && !contains(inputs.sdk, '!all'))) && !contains(inputs.sdk, '!python')
Copy link
Member

Choose a reason for hiding this comment

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

🤯

This is good to go but let's not make too many more changes like this on top of this one or else we'll end up needing a SAT solver to understand what's going on. I fully appreciate that GHA have limited abstraction capacity so this may be needed...

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need a SAT solver... but I did build this to verify the logic:

def contains(haystack: str, needle: str) -> bool:
    return needle in haystack

class input:
    def __init__(self, sdk):
        self.sdk = sdk

def check(query: str, sdk: str) -> bool:
    inputs = input(sdk)
    return eval(query.replace("||", "or").replace("&&", "and").replace(" !", " not "))

check("(contains(inputs.sdk, 'nodejs') || (contains(inputs.sdk, 'all') && !contains(inputs.sdk, '!all'))) && !contains(inputs.sdk, '!nodejs')", "!all")

We've shot ourselves in the foot several times with arrays vs strings through composite actions. It seems like everything is stringly typed given enough transformations. I don't think we will need to make any more changes here. 🤞

I might be able to refactor so the check only needs to show up once per language.

This makes it much easier to read the composite action, and reduces the need to include
the (increasingly complicated) `if:` check in each step. I'm not 100% sure this will work,
and I'm not aware of a way to test. I'm going to push this, then check that it works as
expected.
Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

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

I really like the refactor into separate language composites on principle. 👍

I also think using a comma-separated list is a UX improvement for users, and the updated documentation is fabulous.

However, while they clearly work, I am not super comfortable with the string checks using the ! character in its string form; and the order of logic operations also feels a bit brittle from a maintainer perspective (again, thank you for the excellent comment in the code).

Could it not be also acceptable to provide documentation for using strategy.matrix and using matrix.language under with.sdk?

It's somewhat six of one and half a dozen of the other, just thought I'd throw it out there that we have options for using the current form of the code.


```yaml
steps:
- name: Publish nodejs SDK
uses: pulumi/pulumi-package-publisher@0058a106b68d8277f17bbea0cd29b2ff6e671adc
with:
sdk: nodejs
- name: Publish Java SDK
- name: Publish Java and Python SDKs
uses: pulumi/pulumi-package-publisher@0058a106b68d8277f17bbea0cd29b2ff6e671adc
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking: probably will want to remove the explicit version tagging here and elsewhere, or use @main

@iwahbe
Copy link
Member Author

iwahbe commented Dec 27, 2023

Could it not be also acceptable to provide documentation for using strategy.matrix and using matrix.language under with.sdk?

It's somewhat six of one and half a dozen of the other, just thought I'd throw it out there that we have options for using the current form of the code.

I'm trying to allow opting out of a specific language, without needing to name all other languages. Otherwise, I would agree that using matrix would be equivalent (and simpler for us to maintain).

iwahbe added a commit to pulumi/pulumi-local that referenced this pull request Dec 28, 2023
This change is temporary, and will be removed in the next workflow update.
iwahbe added a commit to pulumi/pulumi-local that referenced this pull request Dec 28, 2023
This change is temporary, and will be removed in the next workflow update.
iwahbe added a commit to pulumi/pulumi-local that referenced this pull request Dec 28, 2023
SHA: 2a65e2577f65a985f0390acc360688adc1125c00
iwahbe added a commit to pulumi/pulumi-local that referenced this pull request Dec 28, 2023
SHA: 2a65e2577f65a985f0390acc360688adc1125c00
@iwahbe iwahbe force-pushed the iwahbe/enable-lang-include-exclude branch from 2a65e25 to 357480c Compare December 28, 2023 09:46
@iwahbe
Copy link
Member Author

iwahbe commented Dec 28, 2023

Factoring out each language cleanly is impossible because of actions/runner#895. I'm trying a workaround: e0cb8d5.

@iwahbe iwahbe force-pushed the iwahbe/enable-lang-include-exclude branch from e0cb8d5 to 9096fb9 Compare December 28, 2023 10:54
@iwahbe iwahbe force-pushed the iwahbe/enable-lang-include-exclude branch from 9096fb9 to 574fc25 Compare December 28, 2023 11:08
@iwahbe iwahbe force-pushed the iwahbe/enable-lang-include-exclude branch from 122986c to a8cbd70 Compare December 28, 2023 11:52
@iwahbe
Copy link
Member Author

iwahbe commented Dec 28, 2023

I have the factored version working 🎉 (example).

@iwahbe iwahbe merged commit bf435b0 into main Dec 28, 2023
1 check passed
@iwahbe iwahbe deleted the iwahbe/enable-lang-include-exclude branch December 28, 2023 13:38
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.

3 participants