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

Add suffix input and support empty strings in prefix/suffix #333

Merged
merged 6 commits into from
Feb 27, 2024

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Feb 15, 2024

Adds a suffix input and supports specifying an empty string element in a list of prefixes/suffixes via either prefix or suffix input by specifying "" instead.

Having suffix next to prefix is probably generally useful, but I have a use case for jupyterhub/jupyter-remote-desktop-proxy to publish the same image with the suffixes "" and "-tiger" as planned via jupyterhub/jupyter-remote-desktop-proxy#88.

This is an example of how I'd use it there:

        # ...
        with:
          prefix: |
            quay.io/jupyterhub/jupyter-remote-desktop-proxy:
          suffix: |
            ""
            -tigervnc
          # ...

@consideRatio consideRatio force-pushed the pr/add-suffix-input branch 2 times, most recently from c471618 to e314ed0 Compare February 15, 2024 17:57
@consideRatio consideRatio reopened this Feb 15, 2024
@consideRatio consideRatio marked this pull request as ready for review February 15, 2024 18:00
@consideRatio consideRatio changed the title Add suffix input and separate prefix/suffix using new lines Add suffix input and support empty strings in prefix/suffix Feb 15, 2024
@consideRatio
Copy link
Member Author

@manics sorry for pinging for review and pushing changes after, I won't touch this further now though.

@manics
Copy link
Member

manics commented Feb 20, 2024

The idea seems fine. Have you thought about using "" to mean the empty string?

@consideRatio
Copy link
Member Author

consideRatio commented Feb 20, 2024

Have you thought about using "" to mean the empty string?

Yes! I found an issue is that we had a whitespace and/or comma deliminated list for prefix, so doing that would be weird there. I looked to use the same syntax for suffix/prefix, and to not make a breaking change. I also concluded that if i made use of new lines as separators for suffix, it would end up making users write hard-to-write-correctly and hard-to-understand yaml config.

@manics
Copy link
Member

manics commented Feb 20, 2024

I meant (basically) '""' not just "". So

          suffix: |
            ""
            -tigervnc
          suffix: "", -tigervnc

@consideRatio
Copy link
Member Author

Ah, okay - we can do that also! I'll update this PR to reflect that when i'm at a computer next time!

@consideRatio
Copy link
Member Author

@manics done!

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@consideRatio
Copy link
Member Author

Thank you @manics, great catches!

@consideRatio consideRatio reopened this Feb 20, 2024
@consideRatio
Copy link
Member Author

Force pushed away repeated auto-format and update dist stuff commits.

@manics manics merged commit 755a93c into jupyterhub:main Feb 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants