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

Skip update checks for bundled packages #91

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

confused-Techie
Copy link
Member

This PR allows PPM to skip checking for updates for bundled packages within Pulsar.

This only works if the package is not a valid git repository, which is true for the vast majority of our bundled packages.

Although, some outliers to this exist, those can be determined by checking the remaining items here. As for all other of the 81 bundled packages, this should allow them to be silently skipped. Assumed to always be up to date by PPM.

This is done by checking the repository of the package.json of the package and determining if it's the same as the repository of Pulsar itself.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

As a reviewer for this repo:

I'm sort of surprised none of the tests complain about this, although I guess exercising this specific scenario is outside the scope of any tests at the moment.

Ideally we might want to add a test for this, maybe as a follow-up perhaps, but I'm not feeling like asking for that for an approve on this, personally. If you want to, that'd be cool.

As a reviewer for pulsar-edit org in general:

Ideally we revert this as soon as we find a way to not ask ppm for these updates for core packages?? Since that seems like a cleaner location for the change to go, over in the requesting-the-check side, not the implementing-the-check side.

EDIT: Maybe this PR you just opened does what I was hoping we'd do instead?: https://github.com/pulsar-edit/pulsar/pull/711/files

For now, I think this is worth doing, and this is a small change scoped specifically to our in-house repos. (We don't generate that repository URL in new package templates, right??)

Alrighty, so with that said, approved. 👍

@DeeDeeG
Copy link
Member

DeeDeeG commented Sep 8, 2023

If you wanna address some of the feedback, then I can wait, otherwise, I am inclined to merge.

(I'm feeling a little move-fast about this PR today myself, but any efforts to arrive at even better approach is appreciated and I'd wait however long it'd take for it if you were so inclined to find improvements along those lines as I mentioned. But they are optional, since this is kind of a hack and workaround anyway. The only alternative approaches we might find could still end up being various degrees of hacks/workarounds.)

And once I review whatever all else has been committed since ppm was synced to core repo, I (or someone else if I can't today) can do a ppm bump in core repo.

@confused-Techie
Copy link
Member Author

@DeeDeeG So on what you've mentioned here.

My other PR over in pulsar doesn't address what you mentioned.

The difficult part about just not asking ppm to check for updates of certain packages, is that settings-view actually doesn't ask for updates to any specific packages.

Instead settings-view just runs the generic command to ask PPM to check for updates for all installed packages. Which would include our bundled ones. So really this may be the only way to stop this behavior from occurring.

As for adding a test, I'd be more than happy to take a look to see if we can do that. But I'd like to say, I don't see any way we can easily revert this PR later down the road without significant changes.

@DeeDeeG
Copy link
Member

DeeDeeG commented Sep 8, 2023

The difficult part about just not asking ppm to check for updates of certain packages, is that settings-view actually doesn't ask for updates to any specific packages.

@confused-Techie so there's no obvious way to check the repository of "all packages" in this instance and not ask for the update?

Is it really asking ppm to "go check all the packages at once to see if upgrades are needed?" Oh, wait... Yes it is.

% ppm upgrade --help

Usage: ppm upgrade
       ppm upgrade --list
       ppm upgrade [<package_name>...]

Upgrade out of date packages installed to ~/.pulsar/packages

This command lists the out of date packages and then prompts to install
available updates.

[ . . . snipped . . . ]

Okay, I see why it wouldn't be easy to do this part over at core repo, and why we have to fix it in two different places, since we're fetching updates for packages for two separate purposes, in two separate ways across settings-view package.

Thanks for the clarification nudging me to check closer what command this PR is editing, I understand better now.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

CI is green with the new test, which is a little above and beyond for this kind of quick fix, even if I did request it. Thank you.

And thank you for the context replying to my previous review comments.

Tests pass (including new test), LGTM! Re-approving.

EDIT to add: And let's merge! 🚀

@confused-Techie
Copy link
Member Author

@DeeDeeG Exactly, which is unfortunate. But yeah settings-view doesn't even know what packages are being checked. So it'd be a significant code change to even make it aware of the packages to them go ahead and not check them. But hopefully this fix isn't too hacky to remain in the codebase for a bit of time.

If we really worried about it's implementation, we could add a flag to the command, such as --skip-core which would optionally enable this feature, so that it worked here, but otherwise wouldn't provide any strange behavior for end users

@confused-Techie
Copy link
Member Author

Lets merge, woot!

@confused-Techie confused-Techie merged commit de6a852 into master Sep 8, 2023
11 checks passed
@confused-Techie confused-Techie deleted the no-update-check-on-core-packages branch September 8, 2023 04:12
@DeeDeeG
Copy link
Member

DeeDeeG commented Sep 15, 2023

On closer look, this ppm upgrade command is already checking only packages under ~/.pulsar/packages. So, bundled packages should be skipped right out of the gate, so to speak. (And it seems to be already not hitting the backend API for packages installed via ppm link or installed from git URLs either, at least in testing on my machine.)

So, even without this PR, ppm upgrade should already be ignoring any truly bundled-with-the-editor packages already, and most (all?) ways folks would get a package with repository matching our core repo URL.

Furthermore, with familiarity of how the packages' package.jsons are formatted on-disk in Pulsar, this seems to be another case where packages get the object form of repository, where repository.url is the real URL we are looking for, and it ends in .git.

For example, when I run ./bin/ppm upgrade --verbose, it is only trying to check for upgrades to my two non-bundled, non-ppm linked, installed packages from the Pulsar package registry.

With some console.log() peppered into the code:

% ./bin/ppm outdated           
NOTTTT skippinggggg
pack.repository is: 
{ type: 'git', url: 'git+https://github.com/dracula/atom.git' }
NOTTTT skippinggggg
pack.repository is: 
{ type: 'git', url: 'git+https://github.com/dracula/atom-ui.git' }
Package Updates Available (0)
└── (empty)
Verbose output (click to expand):
% ./bin/ppm outdated --verbose
NOTTTT skippinggggg
pack.repository is: 
{ type: 'git', url: 'git+https://github.com/dracula/atom.git' }
NOTTTT skippinggggg
pack.repository is: 
{ type: 'git', url: 'git+https://github.com/dracula/atom-ui.git' }
REQUEST {
  url: 'https://api.pulsar-edit.dev/api/packages/dracula-syntax',
  json: true,
  strictSSL: true,
  headers: { 'User-Agent': 'npm/6.14.19-pulsar1-1 node/v16.0.0 darwin x64' },
  callback: [Function (anonymous)],
  method: 'GET'
}
REQUEST {
  url: 'https://api.pulsar-edit.dev/api/packages/dracula-ui',
  json: true,
  strictSSL: true,
  headers: { 'User-Agent': 'npm/6.14.19-pulsar1-1 node/v16.0.0 darwin x64' },
  callback: [Function (anonymous)],
  method: 'GET'
}
REQUEST make request https://api.pulsar-edit.dev/api/packages/dracula-syntax
REQUEST make request https://api.pulsar-edit.dev/api/packages/dracula-ui
REQUEST onRequestResponse https://api.pulsar-edit.dev/api/packages/dracula-syntax 200 {
  'content-type': 'application/json; charset=utf-8',
  vary: 'Accept-Encoding',
  'x-powered-by': 'Express',
  'x-ratelimit-limit': '600',
  'x-ratelimit-remaining': '599',
  'x-ratelimit-reset': '1694789635',
  'ratelimit-limit': '600',
  'ratelimit-remaining': '599',
  'ratelimit-reset': '95',
  etag: 'W/"1a0a-Q4SF5B2tJH9bgIKaZSXrQTJWgJg"',
  'x-cloud-trace-context': 'd488d9ef7af730ba45be48e93e2c6f03',
  date: 'Fri, 15 Sep 2023 14:52:19 GMT',
  server: 'Google Frontend',
  'content-length': '6666',
  connection: 'close'
}
REQUEST reading response's body
REQUEST finish init function https://api.pulsar-edit.dev/api/packages/dracula-syntax
REQUEST response end https://api.pulsar-edit.dev/api/packages/dracula-syntax 200 {
  'content-type': 'application/json; charset=utf-8',
  vary: 'Accept-Encoding',
  'x-powered-by': 'Express',
  'x-ratelimit-limit': '600',
  'x-ratelimit-remaining': '599',
  'x-ratelimit-reset': '1694789635',
  'ratelimit-limit': '600',
  'ratelimit-remaining': '599',
  'ratelimit-reset': '95',
  etag: 'W/"1a0a-Q4SF5B2tJH9bgIKaZSXrQTJWgJg"',
  'x-cloud-trace-context': 'd488d9ef7af730ba45be48e93e2c6f03',
  date: 'Fri, 15 Sep 2023 14:52:19 GMT',
  server: 'Google Frontend',
  'content-length': '6666',
  connection: 'close'
}
REQUEST end event https://api.pulsar-edit.dev/api/packages/dracula-syntax
REQUEST has body https://api.pulsar-edit.dev/api/packages/dracula-syntax 6666
REQUEST emitting complete https://api.pulsar-edit.dev/api/packages/dracula-syntax
REQUEST onRequestResponse https://api.pulsar-edit.dev/api/packages/dracula-ui 200 {
  'content-type': 'application/json; charset=utf-8',
  vary: 'Accept-Encoding',
  'x-powered-by': 'Express',
  'x-ratelimit-limit': '600',
  'x-ratelimit-remaining': '598',
  'x-ratelimit-reset': '1694789635',
  'ratelimit-limit': '600',
  'ratelimit-remaining': '598',
  'ratelimit-reset': '95',
  etag: 'W/"672-IQ59bu7SWhblVpHuXNh89mXJzyI"',
  'x-cloud-trace-context': '1e0d859733f76f54e9d0c3e124bd73df',
  date: 'Fri, 15 Sep 2023 14:52:19 GMT',
  server: 'Google Frontend',
  'content-length': '1650',
  connection: 'close'
}
REQUEST reading response's body
REQUEST finish init function https://api.pulsar-edit.dev/api/packages/dracula-ui
REQUEST response end https://api.pulsar-edit.dev/api/packages/dracula-ui 200 {
  'content-type': 'application/json; charset=utf-8',
  vary: 'Accept-Encoding',
  'x-powered-by': 'Express',
  'x-ratelimit-limit': '600',
  'x-ratelimit-remaining': '598',
  'x-ratelimit-reset': '1694789635',
  'ratelimit-limit': '600',
  'ratelimit-remaining': '598',
  'ratelimit-reset': '95',
  etag: 'W/"672-IQ59bu7SWhblVpHuXNh89mXJzyI"',
  'x-cloud-trace-context': '1e0d859733f76f54e9d0c3e124bd73df',
  date: 'Fri, 15 Sep 2023 14:52:19 GMT',
  server: 'Google Frontend',
  'content-length': '1650',
  connection: 'close'
}
REQUEST end event https://api.pulsar-edit.dev/api/packages/dracula-ui
REQUEST has body https://api.pulsar-edit.dev/api/packages/dracula-ui 1650
REQUEST emitting complete https://api.pulsar-edit.dev/api/packages/dracula-ui
Package Updates Available (0)
└── (empty)

(And it does have some support for checking for updates for packages from git URLs, but these are handled with git commands and fetching from the packages' respective repos, not hitting the package backend API server.)

@DeeDeeG
Copy link
Member

DeeDeeG commented Sep 15, 2023

I dunno if we want to revert the change, considering this is such a small and not-hurting-anything change, but it also isn't doing the thing it's meant to be doing in real-world testing, I think. (Given some further research into things since this was merged.)

Sorry to be a bother about it, I just noticed and wanted to share what I found.

@confused-Techie
Copy link
Member Author

@DeeDeeG Great catch here, and now that I think about it, you are totally right. This wouldn't ever see any packages that are bundled.

So yeah this is kinda pointless, and while harmless, I'd hate for it to add uncertainty into how PPM behaves to someone reading it in the future. So seems best to revert this PR.

@DeeDeeG
Copy link
Member

DeeDeeG commented Sep 17, 2023

There's a shiny "Revert" button next to the "merge" event if scrolling up the history of this PR.

I'm somewhat curious to try it. Hmmmmmmmmmmmm. I might try that in a bit if no objections, or if you want to try it, go for it.

EDIT: Ah. When hovered, it says it will "Create a new pull request to revert these changes". So, nothing too crazy. Makes sense.

@confused-Techie
Copy link
Member Author

@DeeDeeG I've never seen this option. So that sounds like a plan then, lets give it a wirl

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.

2 participants