Remove custom check for CI in favor of using Fastlane's one #336
Conversation
| # Fastlane::is_ci doesn't work here, so reimplementing the code has been necessary. | ||
| # (This should be updated if we change CI or if fastlane is updated.) | ||
| return File.read(secrets_repository_file_path) unless self.encrypt || ENV.key?('CIRCLECI') | ||
| return File.read(secrets_repository_file_path) unless self.encrypt || FastlaneCore::Helper.is_ci? |
There was a problem hiding this comment.
Any idea why the comment initially said that "Fastlane::is_ci? doesn't work here "?
There was a problem hiding this comment.
I'm pretty sure the reason is that is_ci is a Fastlane action and can't be called directly.
If we put it in here, we get a no method error.
There was a problem hiding this comment.
Ah, right. As opposed to calling the helper directly as you did here 👍👌
| # Fastlane::is_ci doesn't work here, so reimplementing the code has been necessary. | ||
| # (This should be updated if we change CI or if fastlane is updated.) | ||
| return File.read(secrets_repository_file_path) unless self.encrypt || ENV.key?('CIRCLECI') | ||
| return File.read(secrets_repository_file_path) unless self.encrypt || FastlaneCore::Helper.is_ci? |
There was a problem hiding this comment.
See is_ci? source here.
To test this change, I run the tests as usual first, the re-run them for this file with CI=1:
CI=1 bundle exec rspec spec/file_reference_spec.rb
I got the following failure:
1) Fastlane::Configuration::FileReference without encryption #source_contents gets the contents from the secrets repo
Failure/Error: expect(subject.source_contents).to eq('source contents')
expected: "source contents"
got: nil
(compared using ==)
That's exactly the failure I would expect to get: is_ci? returned true and we didn't read from the stubbed local secrets but tried to read the encrypted source, which didn't exist.
There was a problem hiding this comment.
That also means that the test is failing on CI though 😓😛
There was a problem hiding this comment.
🤦♂️ is not enough of a reaction...
Generated by 🚫 Danger |
| set_circle_env(true) do | ||
| allow(File).to receive(:read).with(subject.secrets_repository_file_path).and_return('source contents') | ||
| expect(subject.source_contents).to eq(nil) | ||
| end | ||
| allow(FastlaneCore::Helper).to receive(:is_ci?).and_return(true) | ||
| allow(File).to receive(:read).with(subject.secrets_repository_file_path).and_return('source contents') | ||
| expect(subject.source_contents).to eq(nil) |
There was a problem hiding this comment.
Having to work on this made me wonder whether it is appropriate for FileReference to know whether its code runs on CI or not.
I was tempted to leave a FIXME mentioning this and suggesting to move the knowledge up the abstraction ladder, but I didn't because:
FileReferenceis a fat model, not a mere data representation objects. For example, it knows how to diff and apply changes to the file system- This objects is used for the secrets management and, in the long run, we want to use the Rust implementation so I wouldn't recommend working on stuff that is not a necessary bug fix
There was a problem hiding this comment.
Yeah I've never liked the way that this was all hosted in FileReference, this would definitively benefit some restructuration and refactoring… but that's something to keep for later™
Ignoring because this is just a refactor of an internal method. |
To be fair one of the goal of the CHANGELOG is also to be able to, if something stops working with secrets and configure_apply soon after the release, be aware that this might be because of that change here. So that users who notice such a change can look into the CHANGELOG and say "oh, something related to this changed in 3.0.0 so that might be it" and be able to jump in the PR which did the change… 🙃 (I know, we can also use I don't mind skipping it for this one (that's not really a big deal anyway tbh), but good to keep in mind for the future :) |
|
@AliSoftware should I consider your last comment an implicit approval? If so, feel free to hit merge when you read this 😄 Otherwise, let me know what I missed. Thanks. |
Addresses a small
TODOcomment as we prepare for a new release. See also #334 (comment).