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

fix: remove dependency on pyroscope-oss git branch #2143

Merged
merged 12 commits into from
Jul 24, 2023

Conversation

darrenjaneczek
Copy link
Collaborator

@darrenjaneczek darrenjaneczek commented Jul 20, 2023

Now that they live in the same branch on this repository, remove the layer of cross repository dependency on the "og main" branch of "pyroscope-oss" and instead use the subpackages in the new /og subdirectory.

Copy link
Collaborator

@eh-am eh-am left a comment

Choose a reason for hiding this comment

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

👍

Don't forget to update scripts/webpack/webpack.common.ts too! too!

Comment on lines -12 to -17
"@pyroscope/flamegraph/*": [
"./node_modules/pyroscope-oss/packages/pyroscope-flamegraph/*"
],
"@pyroscope/models/*": [
"./node_modules/pyroscope-oss/packages/pyroscope-models/*"
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note these refer to the packages dir, I think they still need to be aliased? Or does the workspaces bit in package.json handles that somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or does the workspaces bit in package.json handles that somehow?

Yes, that appears to be the case. But I am getting the sense that some of the aliases are clashing with the workspaces in certain circumstances. I will continue trying things.

Don't forget to update scripts/webpack/webpack.common.ts too! too!

Thanks for the note. However, my attempts to fix webpack.common.ts are meeting with failure locally.
I am thinking some old "node_modules" imports are still cached, so the current "passed tests" might be a misleading positive.

Copy link
Collaborator Author

@darrenjaneczek darrenjaneczek Jul 20, 2023

Choose a reason for hiding this comment

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

I think I will get rid of the "workspace" entries, and just use aliases at this stage.

@darrenjaneczek
Copy link
Collaborator Author

I will verify this a little more closely tomorrow.
This feels like a good initial step... assuming everything still works.

@darrenjaneczek darrenjaneczek requested a review from eh-am July 20, 2023 23:19
@darrenjaneczek
Copy link
Collaborator Author

@eh-am, the frontend code that is now in "og" has a lot of elaborate testing which I don't suspect has been replicated for the new root of the project. What approach would you recommend for adapting it over to work with the new project root, and all of its overrides?

@darrenjaneczek darrenjaneczek marked this pull request as ready for review July 24, 2023 14:28
@darrenjaneczek darrenjaneczek requested a review from a team as a code owner July 24, 2023 14:28
@darrenjaneczek darrenjaneczek enabled auto-merge (squash) July 24, 2023 21:38
Copy link
Contributor

@Rperry2174 Rperry2174 left a comment

Choose a reason for hiding this comment

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

lgtm!

@darrenjaneczek darrenjaneczek merged commit b552ed5 into next Jul 24, 2023
@darrenjaneczek darrenjaneczek deleted the chore/remove-pyroscope-oss-git-dependency branch July 24, 2023 23:03
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