Use fsinfo for cockpit.file.watch() #20017
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
jelly
commented
Feb 13, 2024
•
jelly
commented
- Machines PR succeeds Makefile: test cockpit.file.watch now using fsinfo cockpit-machines#1449
jelly
added
the
no-test
jelly
force-pushed
the
cockpit-file-rewrite
branch
2 times, most recently
from
b14f6cf
to
249966d
Compare
jelly
force-pushed
the
cockpit-file-rewrite
branch
2 times, most recently
from
86d9139
to
018fe63
Compare
jelly
commented
Feb 19, 2024
jelly
removed
the
no-test
jelly
force-pushed
the
cockpit-file-rewrite
branch
from
018fe63
to
de8fde1
Compare
|
TestPages.testFrameReload and TestMultiMachine.testFrameReload fail due to a difference of handling a non-existent file. Versus: |
| return self; | ||
| } | ||
|
|
||
| import cockpit from "./cockpit.js"; |
There was a problem hiding this comment.
Uh. That's unusual.
Please explain. :)
If having this at the end of the file is the full extent of your "ugly hack" then I could probably be convinced to live with it... but how does it work?
There was a problem hiding this comment.
Soo I looked up some examples of folks trying to get around circular imports and it seems that loaders/import logic ignores imports at the end as cyclic. Possibly as the bundler first combines everything into one file? Either way this is a hack and we should resolve it.
I just didn't feel like doing a big bang rename of cockpit.js to cockpit.ts and merging the typing navigator? Let's discuss how we intend to integrate this all.
There was a problem hiding this comment.
For the record: we can rename pkg/lib/cockpit.js to cockpit.ts with absolutely no other changes and nothing bad seems to happen.
Then we could just add this there...
Note: one thing that I didn't check is to do the rename and then to try to use cockpit from a TS-using project (like navigator) which carries its own cockpit.d.ts file. I wonder if that would make tsc get angry...
jelly
force-pushed
the
cockpit-file-rewrite
branch
from
de8fde1
to
5c789bd
Compare
|
@martinpitt There are two open issues in this PR for which I'd like your opinion on:
Ideally cockpit.file would get a Also there was talk about fsinfo reading small files, possibly a great addition for cockpit.file.watch() like for watching /etc/hostname or crypto policies. So maybe we should wait on that first before declaring public. |
martinpitt
requested changes
Feb 21, 2024
There was a problem hiding this comment.
Thanks! Some minor cleanups, but this is nice enough to land it even with the import hack IMHO.
Wrt. your questions above:
-
I don't like moving it here untyped, that feels like a step backwards. My preferred route would be to land this PR, then send a new one which renames cockpit.js to cockpit.ts (which according to Lis should be fine, except that we need to install and integrate
tscof course). Then you can re-integrate fsinfo.ts, or rearrange things differently to not require circular imports (but the latter is a huge task, let's not do that now). -
I'm fine with declaring fsinfo "stable" now, but @allisonkarlitskaya is the one who has the last word on that. However, I don't see how that needs to be done for this PR -- all that it does is to move
cockpit.file().watch()over to it, and we can adjust that in the future if necessary. -
Agreed that cockpit.file().info() would be nice as next step, but not a blocker for navigator I think.
This was also my idea more of less. Plus at least typing the basics of Cockpit maybe with any?
Well it does make a pkg/lib/cockpit-fsinfo.ts so one could see that as "stable".
👍 For now in navigator we vendor fsinfo.ts, but yeah we should declare this properly and write user docs, QUnit tests. All at least some work. |
You mean because projects grab a particular SHA of pkg/lib, and if we change the bridge it would break. That's a good point. So if we do that, the bridge fsinfo channel needs to remain backwards compatible at least for the (rather limited) API that cockpit.file.watch() uses -- i.e. So one thing about this: |
jelly
force-pushed
the
cockpit-file-rewrite
branch
from
5c789bd
to
7203112
Compare
I agree with everything in this comment, including "next steps".
Can't vendor it if it isn't "upstream" yet 😅 |
martinpitt
previously approved these changes
Feb 21, 2024
| const watch = file.watch((content, tag) => { | ||
| assert.equal(content, null, "non-existant because read is false"); | ||
| assert.notEqual(tag, null, "non empty tag"); | ||
| assert.equal(tag.startsWith("1:"), true, "tag always stars with 1:"); |
|
|
|
Blocked as we get an error now and close the channel when you are in limited access and try to watch
|
jelly
added
the
no-test
jelly
force-pushed
the
cockpit-file-rewrite
branch
from
7203112
to
973e92e
Compare
jelly
force-pushed
the
cockpit-file-rewrite
branch
from
973e92e
to
5ba83f9
Compare
|
Only OSTree based images have a suspicious issue, where the accounts page does not load when we initially login and go to /users/#newuser. Pressing F5 makes the page load. So this seems to be fatal? More interesting obversations: SO HAH! It seems that the first time we login we get a sudo lecture and this somehow fails the bridge, the second time we get no lecture and all is good. |
|
So with the sudo lecture theory, I removed all the state in |
jelly
force-pushed
the
cockpit-file-rewrite
branch
from
3c1d1c1
to
1730342
Compare
jelly
removed
needswork
no-test
jelly
force-pushed
the
cockpit-file-rewrite
branch
from
1730342
to
b2d717a
Compare
|
Adding a close eventlistener and when it first on first load we get close events: On refresh we get zero close events. |
By using the new fsinfo channel fswatch1 can now read the tag without
reading the file when adding a new watch. As side-effect this no longer
reads the full file when `{ read: false }` is passed to `watch()`.
Closes: cockpit-project#19983
Co-Authored-By: Allison Karlitskaya <allison.karlitskaya@redhat.com>
jelly
force-pushed
the
cockpit-file-rewrite
branch
from
528d77f
to
54f5866
Compare
|
I think I'd like to see a new test for anything that uses this. The expiry test is an obvious choice since it's already causing us all this trouble... The test should establish the watch when logged in as the superuser and then de-authenticate. That will break the watch, which ought to be reestablished at that point. This will involve implementing handling of the channel closing unexpectedly. |
This feels like new behavior to me, I can imagine we should do this but this feels like (another) follow up. |
allisonkarlitskaya
requested changes
Apr 5, 2024
There was a problem hiding this comment.
This feels like new behavior to me, I can imagine we should do this but this feels like (another) follow up.
I mean, the entire thing is new behaviour, right?
You took some quick-and-dirty stuff I wrote for the purpose of one specific semi-experimental use-case in an unreleased piece of software (cockpit-files) and are importing it into cockpit now. The standard is definitely going to be higher...
There are definitely unanswered questions here, and top among them (and a point I've raised more than a couple of times) is the need to handle the channel closing...
I wrote the original code there and I'm happy to help with robustifying it, but we definitely still have some things to think through. We managed to "fix" the tests here hard enough that everything goes green, but did we even try running tests against all the other projects?
martinpitt
requested changes
Apr 5, 2024
| channel.close(); | ||
| } | ||
|
|
||
| function effect(callback: (state: FileInfoState) => void) { |
There was a problem hiding this comment.
I won't fight very hard for this, but at least a little: Let's please not call this effect(), it's super confusing as an API. The standard JS approach is to make this an EventTarget and attach a changed or similar event handler to it.
There was a problem hiding this comment.
My 2¢:
- I hate the event mixin API
- we have a lot of other areas in cockpit.js which implement their own callback list mechanism. We even have helper functions for it.
- The name
effect()is definitely suspect, as is the very react-flavoured API (returning the unregister callable). - Maybe we should change it anyway, just for uniformity with everything else in
cockpit.js...
There was a problem hiding this comment.
we have a lot of other areas in cockpit.js which implement their own callback list mechanism
Yeah, but I wouldn't exactly call cockpit.js a role model for good/modern JS. It is really bad from today's POV, it was created pre-ES5 even, with jquery.
There was a problem hiding this comment.
So this seems to be the only real blocker? I am happy to rewrite .effect into something else the question is.. what is a Modern JS API :)
(I am kinda inclined to make it like cockpit.js)
There was a problem hiding this comment.
But should we then just move this into cockpit? The issue is kinda how we then expose fsinfo, cockpit.file.info?
| self.login_and_go("/users", user="scruffy") | ||
| self.login_and_go("/users", user="scruffy", superuser=False) |
There was a problem hiding this comment.
This should be a separate commit with an explanation. It took you so long to figure out, let's please explain it in the commit message for our future selves 😁
There was a problem hiding this comment.
I don't like this change -at all-, and I absolutely veto it. It might be right for this test, but we're doing this to cover up a bug. At the same time, I agree that it's not directly related to this particular test scenario.
My proposal: in the same commit that "fixes" this testcase, we introduce a separate test case which tests this. I'm happy to have it marked as an xfail at first with a link to #20263 and (maybe or maybe not) #20262.
There was a problem hiding this comment.
I'm happy to add an expected failing test, I assume you mean integration not pytest?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.