-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Use fsinfo for cockpit.file.watch() #20017
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,137 @@ | ||
"use strict"; | ||
|
||
function is_json_dict(value: cockpit.JsonValue): value is cockpit.JsonObject { | ||
martinpitt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return value?.constructor === Object; | ||
} | ||
|
||
/* RFC 7396 — JSON Merge Patch — functional */ | ||
function json_merge(current: cockpit.JsonValue, patch: cockpit.JsonValue): cockpit.JsonValue { | ||
if (is_json_dict(patch)) { | ||
const updated = is_json_dict(current) ? { ...current } : { }; | ||
|
||
for (const [key, value] of Object.entries(patch)) { | ||
if (value === null) { | ||
delete updated[key]; | ||
} else { | ||
updated[key] = json_merge(updated[key], value); | ||
} | ||
} | ||
|
||
return updated; | ||
} else { | ||
return patch; | ||
} | ||
} | ||
|
||
interface FileInfo { | ||
type?: string; | ||
tag?: string; | ||
mode?: number; | ||
size?: number; | ||
uid?: number; | ||
user?: string | number; | ||
gid?: number; | ||
group?: string | number; | ||
mtime?: number; | ||
content?: string; | ||
target?: string; | ||
entries?: { | ||
[filename: string]: FileInfo; | ||
}; | ||
targets?: { | ||
[filename: string]: FileInfo; | ||
}; | ||
} | ||
|
||
interface FsInfoError { | ||
problem?: string; | ||
message?: string; | ||
errno?: string; | ||
} | ||
|
||
interface FileInfoState { | ||
info: FileInfo | null; | ||
error: FsInfoError | null; | ||
} | ||
|
||
interface FsInfoHandle { | ||
close(): void; | ||
effect(callback: ((state: FileInfoState) => void)): void; | ||
entry(name: string): FileInfo | null; | ||
state: FileInfoState; | ||
target(name: string): FileInfo | null; | ||
} | ||
|
||
export function fsinfo(path: string, attrs: string[], options?: cockpit.JsonObject) { | ||
const self: FsInfoHandle = { | ||
close, | ||
effect, | ||
entry, | ||
state: { | ||
info: null, | ||
error: null, | ||
}, | ||
target, | ||
}; | ||
|
||
const callbacks: ((state: FileInfoState) => void)[] = []; | ||
|
||
function close() { | ||
channel.close(); | ||
} | ||
|
||
function effect(callback: (state: FileInfoState) => void) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I won't fight very hard for this, but at least a little: Let's please not call this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My 2¢:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this seems to be the only real blocker? I am happy to rewrite (I am kinda inclined to make it like cockpit.js) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But should we then just move this into cockpit? The issue is kinda how we then expose fsinfo, |
||
callback(self.state); | ||
callbacks.push(callback); | ||
return () => callbacks.splice(callbacks.indexOf(callback), 1); | ||
} | ||
|
||
function entry(name: string): FileInfo | null { | ||
return self.state.info?.entries?.[name] ?? null; | ||
} | ||
|
||
function target(name: string): FileInfo | null { | ||
const entries = self.state.info?.entries ?? {}; | ||
const targets = self.state.info?.targets ?? {}; | ||
|
||
let entry = entries[name] ?? null; | ||
for (let i = 0; i < 40; i++) { | ||
const target = entry?.target; | ||
if (!target) | ||
return entry; | ||
entry = entries[target] ?? targets[target] ?? null; | ||
} | ||
return null; // ELOOP | ||
} | ||
|
||
const channel = cockpit.channel({ | ||
superuser: "try", | ||
payload: "fsinfo", | ||
path, | ||
attrs, | ||
watch: true, | ||
...options | ||
}); | ||
|
||
let state: cockpit.JsonValue = null; | ||
channel.addEventListener("message", (_event: CustomEvent, payload: string) => { | ||
state = json_merge(state, JSON.parse(payload)); | ||
|
||
if (is_json_dict(state) && !state.partial) { | ||
self.state = { | ||
info: is_json_dict(state.info) ? state.info : null, | ||
error: is_json_dict(state.error) ? state.error : null, | ||
}; | ||
|
||
for (const callback of callbacks) { | ||
callback(self.state); | ||
} | ||
} | ||
}); | ||
|
||
return self; | ||
} | ||
|
||
// FIXME: import at the end of the file to prevent circular import build issues | ||
// this is a temporary measure until we move cockpit.js to cockpit.ts in a follow up. | ||
import cockpit from "./cockpit.js"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the record: we can rename 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
martinpitt marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1046,7 +1046,7 @@ class TestAccounts(testlib.MachineCase): | |
b.wait_not_present("#password-expiration") | ||
|
||
b.logout() | ||
self.login_and_go("/users", user="scruffy") | ||
self.login_and_go("/users", user="scruffy", superuser=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy to add an expected failing test, I assume you mean integration not pytest? |
||
b.go("#/scruffy") | ||
b.wait_text("#account-user-name", "scruffy") | ||
b.wait_text("#account-expiration-text", "Never expire account") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
starts ⭐