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

Ensure nodejs path globs prepend all relative path strings with ./ #2861

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

rquitales
Copy link
Contributor

@rquitales rquitales commented Mar 5, 2024

Proposed changes

The latest version of the NodeJS glob package introduced in #2858 has slightly different behaviour than in the previously used version. This PR sets the options for these glob functions to maintain backwards compatibility and ensure our resource urns do not change.

Related issues (optional)

Fixes: #2860

    newest version of this package drops the leading `./` for relative
file paths. This commit updates this behaviour to maintain backwards
compatability.
Copy link

github-actions bot commented Mar 5, 2024

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.46%. Comparing base (e74728b) to head (fd7e8e7).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2861   +/-   ##
=======================================
  Coverage   25.46%   25.46%           
=======================================
  Files          48       48           
  Lines        7532     7532           
=======================================
  Hits         1918     1918           
  Misses       5456     5456           
  Partials      158      158           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rquitales rquitales changed the title Rquitales/fix node sdk Ensure nodejs path globs prepend all relative path strings with ./ Mar 5, 2024
@rquitales rquitales requested review from EronWright and a team March 5, 2024 00:25
Copy link
Contributor

@mjeffryes mjeffryes left a comment

Choose a reason for hiding this comment

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

Thanks for tracing this down! (Does this test not run on PRs? Curious why we didn't see the failure before merge.)

How confident do you feel that this is the only behavior change from the new version of glob? Do we need to do more sanity checking before release?

@rquitales
Copy link
Contributor Author

rquitales commented Mar 5, 2024

Thanks for tracing this down! (Does this test not run on PRs? Curious why we didn't see the failure before merge.)

How confident do you feel that this is the only behavior change from the new version of glob? Do we need to do more sanity checking before release?

The PR did actually fail the presubmit test (see test results in #2858 and link to test failure: https://github.com/pulumi/pulumi-kubernetes/actions/runs/8118521069/job/22193057050). Not sure why automerge merged it in despite the test failure.

I think this should be the only required change for glob settings. We only have 2 code paths that use the glob package, and scanning through the API changes in the changelog indicate we should probably be ok with the use cases in pu/kubernetes.

@rquitales rquitales merged commit 0b393b1 into master Mar 5, 2024
20 checks passed
@rquitales rquitales deleted the rquitales/fix-node-sdk branch March 5, 2024 01:01
lumiere-bot bot added a commit to coolguy1771/home-ops that referenced this pull request Mar 7, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@pulumi/kubernetes](https://pulumi.com)
([source](https://togithub.com/pulumi/pulumi-kubernetes)) | dependencies
| minor | [`4.8.1` ->
`4.9.0`](https://renovatebot.com/diffs/npm/@pulumi%2fkubernetes/4.8.1/4.9.0)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>pulumi/pulumi-kubernetes (@&#8203;pulumi/kubernetes)</summary>

###
[`v4.9.0`](https://togithub.com/pulumi/pulumi-kubernetes/blob/HEAD/CHANGELOG.md#490-March-4-2024)

[Compare
Source](https://togithub.com/pulumi/pulumi-kubernetes/compare/v4.8.1...v4.9.0)

- Fix SSA ignoreChanges by enhancing field manager path comparisons
([pulumi/pulumi-kubernetes#2828)
- Update nodejs SDK dependencies
([pulumi/pulumi-kubernetes#2858,
[pulumi/pulumi-kubernetes#2861)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMjcuMSIsInVwZGF0ZWRJblZlciI6IjM3LjIyNy4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
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.

Workflow failure: build
2 participants