pkg: drop "version" from manifests #15396
Conversation
allisonkarlitskaya
force-pushed
the
remove-manifest-version
branch
from
9cd5583
to
7635e65
Compare
|
Several failures, moving to draft. I also fixed the PR description. |
|
The tests work again when I just revert pkg/base1/manifest.json.in , i.e. put back VERSION there. That seems to be the only relevant one. I can't immediately pinpoint code that would read it, though. |
|
login page's machine_application_login_reload() looks at the base1 manifest version. This is ancient code which we really don't need any more. I removed that code locally and at least the handful of tests that I ran locally work fine now. |
|
With this patch most of the failing tests should work again, but locally I still get a failure with |
martinpitt
force-pushed
the
remove-manifest-version
branch
from
7635e65
to
da36b1b
Compare
|
Force-pushed to include that commit, let's see what's left now. In the worst case, we just keep the version in base1 and teach |
|
I split out dropping the compat code to #15423 to get it tested separately. Update: That is all green, and works. |
allisonkarlitskaya
force-pushed
the
remove-manifest-version
branch
from
da36b1b
to
b6fbd83
Compare
|
Looking at the TestSuperuserOldWebserver.test failure, some notes.
@mvollmer, do you know something off the top of your head what could require the bridge manifest's version here? |
|
This reproduces interactively. Clicking the menu on the remote machine on http://127.0.0.2:9091/cockpit+=10.111.113.2/@localhost/shell/index.html most often does not do anything; on some pages it works, but it always takes two or five clicks to eventually succeed. Also, the URL does not change at all. My theory is that this is somehow related to 4511d83 -- while the new (remote) bridge does not have that any more, the old centos-7 bridge certainly would. As such it seems to me that we'd first need to drop that backwards compat code from all the old bridges which are around, which given RHEL 7 will still be a while. My proposal: Put back a static "version: "219" field into base1/manifest.json, but stop treating it as a package version, and consider it a legacy "API version" field for older releases, until RHEL/CentOS 7 go EOL. "219" because that's when the behaviour was last modified in PR #13482. @allisonkarlitskaya , @mvollmer , WDYT? |
I can't say anything about this, unfortunately. Putting a static version just into base1/manifest certainly looks reasonable. |
This field was introduced in cockpit-project#4964 as "purely informational for now", and isn't even parsed by cockpit. Meanwhile, it's one of the few things preventing us from untangling our webpack build from autotools. Even within cockpit, usage is inconsistent: most packages set "version" to the string that @Version@ expands to, but machines had it set to the integer 0, and pcp and static had no "version" field at all. There are only two places where this was being used: - Login pages before commit 4511d83 check the remote base1 manifest version to decide if it supports remote URLs. This version is still supported for a few years in RHEL/CentOS 7, so for now we can't remove base1's version field. But this only functions as an "API compatibility level" field: it does not need to accurately follow the release numbers, it just needs to be bumped on incompatible changes. The last one was the sudo authentication rework in PR cockpit-project#13482 in version 219, so set version to that and add a `version-note:` field (in lieu of a comment) that explains the number. - The base1/test-http.js unit test: this directly reads the manifest.json.in file from the playground and does a direct comparison on it. That's been adjusted accordingly. Thus, drop all version fields except base1's.
martinpitt
force-pushed
the
remove-manifest-version
branch
from
b6fbd83
to
a57302e
Compare
|
Done, with this it ought to succeed now. Marking as ready for review. @mvollmer, want to give it a final review, as both Lis and I touched this now? |
|
This is about as green as tests can get right now, this looks good to land. @mvollmer, can you please review? |
This field was introduced in #4964 as "purely informational for now",
and isn't even parsed by cockpit. Meanwhile, it's one of the few things
preventing us from untangling our webpack build from autotools.
Even within cockpit, usage is inconsistent: most packages set "version"
to the string that @Version@ expands to, but machines had it set to the
integer 0, and pcp and static had no "version" field at all.
It seems nobody is going to miss this. Just drop it.
The one place which did reference this was a test which directly read
the manifest.json.in file from the playground and does a direct
comparison on it. That's been adjusted accordingly now.