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

feat: Add configMap option #645

Merged
merged 27 commits into from
Jul 8, 2022
Merged

feat: Add configMap option #645

merged 27 commits into from
Jul 8, 2022

Conversation

simenandre
Copy link
Contributor

@simenandre simenandre commented Jun 11, 2022

This is exactly the same pull request as #584, but I've removed some of the unrelated changes that needs to come as separate pull requests.

closes #584
fixes #165
needs #651

@pulumi pulumi deleted a comment from pulumi-bot Jun 11, 2022
@simenandre simenandre changed the title small fixup 584 feat: Add configMap option Jun 11, 2022
@simenandre simenandre requested a review from stack72 June 11, 2022 12:31
@Moon1706
Copy link
Contributor

@cobraz , you forgot one important change.
You'll get this error without it. It relates to this issues.

@Moon1706
Copy link
Contributor

Sorry, I've seen that you add this PR. So, merge Node version changes before this PR.

@t0yv0 t0yv0 self-requested a review June 24, 2022 14:26
@t0yv0
Copy link
Member

t0yv0 commented Jun 24, 2022

Merging master after #561 is in.

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

t0yv0 commented Jul 7, 2022

@RobbieMcKinstry were you taking this PR over the finish line? This LGTM basically just needs the lock file conflicts resolved. The map-in-string encoding issue is inevitable it seems with how Actions work.

@RobbieMcKinstry
Copy link
Contributor

@t0yv0 I was going to review if no one else got to it, but you have much greater context on this matter so I'm deferring to you. :D

If you want to hammer-stamp it, I can fix the merge conflict. But I think we should wait until we release v3.18.1 and fix the P1 before merging, since this feature touches a lot of files!

Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

LGTM modulo conflicts.

@RobbieMcKinstry
Copy link
Contributor

I encountered a small problem with this but it shouldn't cause much delay. I went to rebase via the CLI but the branch had diverged pretty far. So I used the GH UI to merge main into this branch and resolve the conflicts there. But one of the conflicting files was yarn.lock... I went ahead and hand-edited it, but it should be autogenerated, that much I know.

I'm just not sure if it should be generated as part of CI using the bot, or if it's generated locally and committed during the PR process. I don't know what the conversion is here and I don't have enough exposure to the Node ecosystem to know the right answer :)

I'm going to look at the CI actions for this repro to figure it out. This was an opportunity for me to learn in the open, albeit 15 minutes at a time :)

@RobbieMcKinstry
Copy link
Contributor

I regenerated the lockfile and committed it. Please let me know if this was the wrong thing to do, and I'll revert.

Looks like the more pressing issue is the nullish chaining operator (x?.y) isn't supported in Node 12. I know I recently added code to fix the P1 that also uses that operator. It makes sense why these tests would fail, now that we've reverted to Node 12. But I'm not sure how v3.18.1 is running if the action uses Node 12, but the syntax is invalid for Node 12!

@t0yv0 what do you think? Is it possible the action could run in an environment later than Node 12? Say, if the user installs a newer version of Node. I don't know how this code here is successfully running in prod; I must be missing something.

@t0yv0
Copy link
Member

t0yv0 commented Jul 8, 2022

Regenerating yarn.lock sounds good yes.

@t0yv0
Copy link
Member

t0yv0 commented Jul 8, 2022

Regarding ? operator maybe typescript compiles it down so it runs as something else on JS node 12?

@t0yv0
Copy link
Member

t0yv0 commented Jul 8, 2022

So @RobbieMcKinstry I'm digging into this to try to understand it. It sounds like dist/ is the compiled JavaScript (from TypeScript). It's unclear whether we should be checking in the compiled version but it sounds like we should. To recompile it we do yarn build. Currently on my machine it generates a diff, and introduces this ? operator into dist/index.js - I can then repro what's happening in CI. So the CI script runs yarn build.

I suspect something changed in the compiler options or version?

@RobbieMcKinstry
Copy link
Contributor

The short answer is that /dist is not meant to be touched by us humans. There's a bot that recompiles it after each PR is merged. The bot is responsible for keeping master up to date: mirroring what's in src with the compiled code in dist.

@RobbieMcKinstry RobbieMcKinstry merged commit 01605fa into master Jul 8, 2022
@pulumi-bot pulumi-bot deleted the small-fixup-584 branch July 8, 2022 22:47
@t0yv0
Copy link
Member

t0yv0 commented Jul 11, 2022

🙇

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.

Support the --config-file feature
7 participants