Skip to content

[GCSI-515] update fastlane build actions android/ios #2

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

Open
wants to merge 6 commits into
base: GCSI-409
Choose a base branch
from

Conversation

Yousif-Ahmed
Copy link
Contributor

No description provided.

Comment on lines 36 to 50
def self.fetch_android_build_path(lane_context)
all_aab_paths = lane_context[Actions::SharedValues::GRADLE_ALL_AAB_OUTPUT_PATHS]
return all_aab_paths if all_aab_paths && !all_aab_paths.empty?

aab_path = lane_context[Actions::SharedValues::GRADLE_AAB_OUTPUT_PATH]
return aab_path if aab_path && !aab_path.empty?

all_apk_paths = lane_context[Actions::SharedValues::GRADLE_ALL_APK_OUTPUT_PATHS]
return all_apk_paths if all_apk_paths && !all_apk_paths.empty?

apk_path = lane_context[Actions::SharedValues::GRADLE_APK_OUTPUT_PATH]
return apk_path if apk_path && !apk_path.empty?

return nil
end

Choose a reason for hiding this comment

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

Suggested change
def self.fetch_android_build_path(lane_context)
all_aab_paths = lane_context[Actions::SharedValues::GRADLE_ALL_AAB_OUTPUT_PATHS]
return all_aab_paths if all_aab_paths && !all_aab_paths.empty?
aab_path = lane_context[Actions::SharedValues::GRADLE_AAB_OUTPUT_PATH]
return aab_path if aab_path && !aab_path.empty?
all_apk_paths = lane_context[Actions::SharedValues::GRADLE_ALL_APK_OUTPUT_PATHS]
return all_apk_paths if all_apk_paths && !all_apk_paths.empty?
apk_path = lane_context[Actions::SharedValues::GRADLE_APK_OUTPUT_PATH]
return apk_path if apk_path && !apk_path.empty?
return nil
end
def self.fetch_android_build_path(lane_context)
build_keys = [
Actions::SharedValues::GRADLE_APK_OUTPUT_PATH,
Actions::SharedValues::GRADLE_AAB_OUTPUT_PATH,
Actions::SharedValues::GRADLE_ALL_APK_OUTPUT_PATHS,
Actions::SharedValues::GRADLE_ALL_AAB_OUTPUT_PATHS
]
build_keys.each do |build_key|
build_path = lane_context[build_key]
return build_path if build_path.presence
end
end

Choose a reason for hiding this comment

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

I thought that APK is more popular than AAB & single-variant is used more than multi-variant?
Is this wrong? Any order is fine with me 3moman...

Choose a reason for hiding this comment

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

Also, why not move this function to the build_android since it won't be used by any other action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure will update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahmedhany98 shouldn't the priority for multi-variant so that if not multi-variant, we should check the single variant ? i thought that
but will double check let me know if u are sure

Choose a reason for hiding this comment

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

I think checking the most commonly used paths earlier in the array is slightly better.
The order won't affect the logic 3moman since they can only have one of the constants. The single-variant value should be undefined in case of multi-variant based on their comment here
We can check for the multi-variant first to follow their structure here.

Comment on lines +112 to +119
FastlaneCore::ConfigItem.new(
key: :instabug_api_base_url,
env_name: "INSTABUG_API_BASE_URL",
description: "Instabug API base URL (defaults to https://api.instabug.com)",
optional: true,
type: String,
skip_type_validation: true # Since we don't extract this param
)

Choose a reason for hiding this comment

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

I don't understand this point.
Should the STs customers pass the env_var as a param? or we will be fetching it directly if exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahmedhany98, it will be an ENV var, which is optional normally
Let me illustrate

  • env vars can not included in available options normally as they aren't params
  • i included it for documentation as when the user checks the available action can found the description for it and the env name
  • so i added skip_type_validation so that the user shouldn't include it as a param and can export it as env var
    as example he only should
    export INSTABUG_API_BASE_URL =url and no need to send it to us as fastlane read the env vars passed to it

Choose a reason for hiding this comment

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

Tmam perfect. Thank you for explaining 🙏

Copy link

@ahmedhany98 ahmedhany98 left a comment

Choose a reason for hiding this comment

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

Great work ya Youssef 👏

]
build_keys.each do |build_key|
build_path = lane_context[build_key]
return build_path if build_path && !build_path.empty?

Choose a reason for hiding this comment

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

Suggested change
return build_path if build_path && !build_path.empty?
return build_path if build_path.present?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants