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

Add pulumi-version option #661

Merged
merged 17 commits into from
Jul 25, 2022
Merged

Add pulumi-version option #661

merged 17 commits into from
Jul 25, 2022

Conversation

lomholdt
Copy link
Contributor

@lomholdt lomholdt commented Jun 21, 2022 •
edited
Loading

This PR adds the pulumi-version option allowing pinning the CLI version that get's installed while running the action.
If the pulumi-version option is not set it will default to ^3 as this was the previously hardcoded value.

jaxxstorm
jaxxstorm reviewed Jun 22, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@simenandre simenandre mentioned this pull request Jun 23, 2022
Merged
@Moon1706

This comment was marked as off-topic.

Sign in to view
@RobbieMcKinstry RobbieMcKinstry self-requested a review July 1, 2022 13:39
@RobbieMcKinstry
Copy link
Contributor

RobbieMcKinstry commented Jul 1, 2022 •
edited
Loading

Hi @lomholdt , thank you for your contribution! As I recall, we have a similar parameter in the Azure DevOps Pipeline CI integration, so adding the Pulumi version sounds like it makes sense here too.

I'm seeing this branch has some conflicts. Would you be able to rebase?

@RobbieMcKinstry RobbieMcKinstry added kind/enhancement Improvements or new features impact/usability Something that impacts users' ability to use the product easily and intuitively area/cicd labels Jul 1, 2022
@RobbieMcKinstry RobbieMcKinstry added this to the 0.75 milestone Jul 1, 2022
RobbieMcKinstry
RobbieMcKinstry reviewed Jul 1, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
RobbieMcKinstry
RobbieMcKinstry suggested changes Jul 1, 2022
View reviewed changes
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@RobbieMcKinstry
Copy link
Contributor

RobbieMcKinstry commented Jul 1, 2022 •
edited
Loading

Also, it looks like there was some discussion on #661 about generating a warning if the CLI version number is incompatible with the automation API version number.
@lomholdt Would you add a warning if the version specified is beneath v3? Thank you very much! :D
Please re-request my review when you've made these changes. I'm going to be away from my computer for a few days for the holiday, but I can take a look when I get back. :)

@RobbieMcKinstry RobbieMcKinstry linked an issue Jul 1, 2022 that may be closed by this pull request
Have an ability to change pulumi cli version #437
Closed
@RobbieMcKinstry RobbieMcKinstry mentioned this pull request Jul 1, 2022
Closed
@jonas-lomholdt
Copy link

Hi @lomholdt , thank you for your contribution! As I recall, we have a similar parameter in the Azure DevOps Pipeline CI integration, so adding the Pulumi version sounds like it makes sense here too.

I'm seeing this branch has some conflicts. Would you be able to rebase?

Sure, I will fix that. Thanks 👍🏼

@jonas-lomholdt
Copy link

Also, it looks like there was some discussion on #661 about generating a warning if the CLI version number is incompatible with the automation API version number.

@lomholdt Would you add a warning if the version specified is beneath v3? Thank you very much! :D

Please re-request my review when you've made these changes. I'm going to be away from my computer for a few days for the holiday, but I can take a look when I get back. :)

Sure, that makes sense. I will give it a shot. Thanks. Have a nice holiday 👍🏼

@RobbieMcKinstry
Copy link
Contributor

Would you also remove the dist file updates from your PR? I believe the TypeScript files are automatically recompiled by a GitHub Action once a commit lands on master/main.

@RobbieMcKinstry RobbieMcKinstry self-assigned this Jul 6, 2022
@RobbieMcKinstry
Copy link
Contributor

I think this PR will also need a rebase, since the CHANGELOG was modified.

@RobbieMcKinstry RobbieMcKinstry mentioned this pull request Jul 11, 2022
Closed
@RobbieMcKinstry
Copy link
Contributor

Hey @jonas-lomholdt, just following up on this. Let me know if you need any assistance with the remaining action items. :)

@lomholdt
Copy link
Contributor Author

lomholdt commented Jul 21, 2022 •
edited
Loading

Hey @jonas-lomholdt, just following up on this. Let me know if you need any assistance with the remaining action items. :)

Hey @RobbieMcKinstry, sorry I have been taking some vacation. I fixed the conflicts and need to warn if the version is less than 3. I would probably need to parse the input string somehow to get a semver representation of it that I can compare. Did you have any specific way in mind? low-fi by string splitting or pulling in a lib that does that? Thanks!

@RobbieMcKinstry
Copy link
Contributor

Hi there @lomholdt I hope you had a lovely vacation! It's well-deserved, I'm sure!
I would personally prefer using a library. I don't have a recommendation. The top Google result looks decent.

@lomholdt
Copy link
Contributor Author

Hi there @lomholdt I hope you had a lovely vacation! It's well-deserved, I'm sure! I would personally prefer using a library. I don't have a recommendation. The top Google result looks decent.

Hey @RobbieMcKinstry. I noticed there's already a semver package installed, and I think that will do the trick. I added a warning if the parsed semver is less than 3.0.0. You probably have an opinion about what the message should say, but I just put something in there for now. Can you give it another look? Thanks!

simenandre
simenandre approved these changes Jul 25, 2022
View reviewed changes
Copy link
Collaborator

@simenandre simenandre left a comment

Choose a reason for hiding this comment

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

Thank you for this!

src/libs/pulumi-cli.ts Show resolved Hide resolved
@RobbieMcKinstry RobbieMcKinstry modified the milestones: 0.75, 0.76 Jul 25, 2022
RobbieMcKinstry
RobbieMcKinstry approved these changes Jul 25, 2022
View reviewed changes
Copy link
Contributor

@RobbieMcKinstry RobbieMcKinstry left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks very much for your contribution!

src/libs/pulumi-cli.ts Outdated Show resolved Hide resolved
@lomholdt
Copy link
Contributor Author

LGTM! Thanks very much for your contribution!

Awesome! Thanks 😃 No problem, and sorry for the delay.

@RobbieMcKinstry
Copy link
Contributor

RobbieMcKinstry commented Jul 25, 2022 •
edited
Loading

No need to apologize at all! I don't consider it a delay; vacations are necessary!

@RobbieMcKinstry RobbieMcKinstry merged commit 43e0b10 into pulumi:master Jul 25, 2022
@RobbieMcKinstry
Copy link
Contributor

Just FYI I don't plan to cut a release until the second week of August, which is when I would expect this change to ship. Not sure if anyone else has plans that supersede mine, though ;D

@lomholdt
Copy link
Contributor Author

Just FYI I don't plan to cut a release until the second week of August, which is when I would expect this change to ship. Not sure if anyone else has plans that supersede mine, though ;D

No problem. When I needed this for testing prior versions I just used my local fork and changed back to this afterwards. Looking forward to the next release 🚀

@lomholdt lomholdt deleted the add-pinned-pulumi-version branch July 25, 2022 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaxxstorm jaxxstorm jaxxstorm left review comments

@simenandre simenandre simenandre approved these changes

@RobbieMcKinstry RobbieMcKinstry RobbieMcKinstry approved these changes

Assignees

@RobbieMcKinstry RobbieMcKinstry

Labels
area/cicd impact/usability Something that impacts users' ability to use the product easily and intuitively kind/enhancement Improvements or new features
Projects
None yet
Milestone
0.76
Development

Successfully merging this pull request may close these issues.

Have an ability to change pulumi cli version
6 participants
@lomholdt @Moon1706 @RobbieMcKinstry @jonas-lomholdt @jaxxstorm @simenandre