Improve Usability of replace-scm-with-registry#9966
Conversation
|
#9854 suggested that this config be added to https://github.com/apple/swift-configuration.
|
|
@swift-ci test |
|
@swift-ci test windows |
|
@swift-ci test macOS |
bripeticca
left a comment
There was a problem hiding this comment.
Overall LGTM, just a couple questions:
- Have we considered that the use of
--replace-scm-with-registryin the CLI in turn populates the registries.json with this value set, or are we treating these as strictly separate concepts where the CLI will override whatever is present in the registries.json? - If the latter, will users have to explicitly declare this property in the
registries.jsonthemselves? - If the CLI option does affect whether the replace scm flag is persisted to the registries.json file, do we write to the registries.json when this happens? Does the
RegistryConfigurationencoding work as expected?
| let json = #""" | ||
| { | ||
| "authentication": {}, | ||
| "replaceScmWithRegistry": "Bob, I don't think this is valid", |
|
@swift-ci test windows |
| // registries.json acts as a default; an explicit CLI flag overrides. | ||
| let effectiveTransformation: WorkspaceConfiguration.SourceControlToRegistryDependencyTransformation = { | ||
| if let cliValue = configuration.sourceControlToRegistryDependencyTransformation { | ||
| return cliValue | ||
| } | ||
| if registriesConfiguration.replaceScmWithRegistry == true { | ||
| return .swizzle | ||
| } | ||
| return .disabled | ||
| }() |
There was a problem hiding this comment.
Overall LGTM, just a couple questions:
--replace-scm-with-registry doesn't write to registries.json. The transformation used is determined by this logic.
CLI takes precedent over what's in registries.json, which takes precedent over the default (which is a .disabled transformation).
Users will have to know to include this new attribute in registries.json. That raises a good question: what's the best way to tell SPM users about this new attribute?
| } | ||
|
|
||
| try container.encode(self.registryAuthentication, forKey: .authentication) | ||
| try container.encodeIfPresent(self.security, forKey: .security) |
There was a problem hiding this comment.
We should also be encoding replaceScmWithRegistry
|
@swift-ci test windows |
|
@swift-ci test mac |
|
@swift-ci test linux |
|
@swift-ci test macOS |
|
@swift-ci test windows |
| XCTAssertThrowsError(try self.decoder.decode(RegistryConfiguration.self, from: json)) | ||
| } | ||
|
|
||
| func testDecodeConfigurationWithMissingReplaceScmWithRegistry() throws { |
There was a problem hiding this comment.
suggestion: consider migrating this test suite to Swift Testing, and utilizing the parameterized tests for test implementation that are similar.
There was a problem hiding this comment.
Does that belong in this PR? What if I add a new migration PR and stack it on top of this one?
There was a problem hiding this comment.
There is no need to convert to Swift Testing in this PR.
There was a problem hiding this comment.
Perfect. I will make a new PR that is blocked by this one.
| """#) | ||
|
|
||
| // Case 1: no CLI override → registries.json drives .swizzle. | ||
| do { |
There was a problem hiding this comment.
suggestion: consider migrating this test suite to Swift Testing, and utilizing the parameterized tests for test implementation that are similar.
|
@swift-ci test self hosted windows |
3 similar comments
|
@swift-ci test self hosted windows |
|
@swift-ci test self hosted windows |
|
@swift-ci test self hosted windows |
Add
replaceScmWithRegistryattribute toregistries.json. Resolves #9854.Motivation:
There is a no global option, or at least per repo that could enable the --replace-scm-with-registry option? This is very error prone to have to setup alias scripts to do use correct flag.
Result:
Devs can now transform SCM dependencies to registry ones globally or on a per-project basis by including
"replaceScmWithRegistry": true,in.swiftpm/configuration/registries.jsonManual Testing
Make a package like the one in
WorkspaceTests.testRegistriesJsonReplaceScmWithRegistryDrivesSwizzle. Make sure that SPM picks up"replaceScmWithRegistry": true,.Explore other edge cases like not including
"replaceScmWithRegistry", passing in an illegal value, etc...Blocked by #10036