Skip to content

Remove custom check for CI in favor of using Fastlane's one #336

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 7, 2022
Merged

Remove custom check for CI in favor of using Fastlane's one #336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a748da6
Remove custom check for CI in favor of using Fastlane's one
mokagio Feb 7, 2022
1f57542
Account for `FastlaneCore::Helper:is_ci?` usage in tests
mokagio Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions lib/fastlane/plugin/wpmreleasetoolkit/models/file_reference.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ def initialize(params = {})
end

def source_contents
# TODO: This works only on CircleCI. I chose this variable because it's the one checked by Fastlane::is_ci.
# 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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea why the comment initially said that "Fastlane::is_ci? doesn't work here "?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. As opposed to calling the helper directly as you did here 👍👌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also means that the test is failing on CI though 😓😛

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ is not enough of a reaction...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

return nil unless File.file?(encrypted_file_path)

encrypted = File.read(encrypted_file_path)
Expand Down
14 changes: 6 additions & 8 deletions spec/file_reference_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,19 +69,17 @@

describe '#source_contents' do
it 'gets the contents from the secrets repo' do
set_circle_env(false) do
allow(File).to receive(:read).with(subject.secrets_repository_file_path).and_return('source contents')
expect(subject.source_contents).to eq('source contents')
end
allow(FastlaneCore::Helper).to receive(:is_ci?).and_return(false)
allow(File).to receive(:read).with(subject.secrets_repository_file_path).and_return('source contents')
expect(subject.source_contents).to eq('source contents')
end
end

describe '#source_contents on ci' do
it 'gets the contents from the secrets repo' do
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)
Comment on lines -81 to +82
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. FileReference is a fat model, not a mere data representation objects. For example, it knows how to diff and apply changes to the file system
  2. 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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™

end
end

Expand Down
18 changes: 0 additions & 18 deletions spec/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,6 @@ module SpecHelper
config.filter_run_when_matching :focus
end

def set_circle_env(define_ci)
is_ci = ENV.key?('CIRCLECI')
orig_circle_ci = ENV['CIRCLECI']
if define_ci
ENV['CIRCLECI'] = 'true'
else
ENV.delete 'CIRCLECI'
end

yield
ensure
if is_ci
ENV['CIRCLECI'] = orig_circle_ci
else
ENV.delete 'CIRCLECI'
end
end

# Allows Action.sh to be executed even when running in a test environment (where Fastlane's code disables it by default)
#
def allow_fastlane_action_sh
Expand Down