Skip to content
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

Add support default.txt changelog name #331

Conversation

denis-tikhomirov
Copy link
Contributor

@denis-tikhomirov denis-tikhomirov commented Mar 24, 2022

Task name: GooglePlayReleaseV4

Description: The changes adds default.txt changelog file to list of required changelog files for uploading.

Documentation changes required: (Y/N) N

Added unit tests: (Y/N) N

Attached related issue: (Y/N) Y

Checklist:

  • Task version was bumped - please check instruction how to do it
  • Checked that applied changes work as expected

Checked test cases:

  1. there is no files in changelogs directory - no one changelog is uploaded with metadata
  2. there is file with name different from version number only - no one changelog is uploaded with metadata
  3. there is file default.txt only - file default.txt is used as changelog
  4. there is file with name equal to version number only - file with name equal to version number is used as changelog
  5. there are 2 files: default.txt and file with name equal to version number - file with name equal to version number is used as changelog

@ghost
Copy link

ghost commented Mar 29, 2022

Tested changes manually. LGTM!

@anatolybolshakov
Copy link
Contributor

@alexander-smolyakov could you please take a look?

@anatolybolshakov
Copy link
Contributor

@denis-tikhomirov please share test scenarios you covered here

} else {
tl.debug(`The name of the file ${changelogFile} is not a valid version code. Skipping it.`);
}
}

if (releaseNotes.length === 0 && fullDefaultChangelogPath !== '') {
const message: string = `'default.txt' release notes file is found. It is used for language code ${languageCode}`;
Copy link
Contributor

@anatolybolshakov anatolybolshakov Mar 30, 2022

Choose a reason for hiding this comment

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

Why it should be required? Could it make sense to have it as optional instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, see it, thanks! It will be used only if there are no other changelogs as default - so it's optional in general

Copy link
Contributor

@anatolybolshakov anatolybolshakov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Please share test results in more details also

@denis-tikhomirov
Copy link
Contributor Author

@denis-tikhomirov please share test scenarios you covered here

Description is updated with checked test cases

Copy link
Contributor

@alexander-smolyakov alexander-smolyakov left a comment

Choose a reason for hiding this comment

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

Could you please take a look at the comments:

@denis-tikhomirov
Copy link
Contributor Author

Could you please take a look at the comments:

Done. @alexander-smolyakov, please review this again.

@denis-tikhomirov
Copy link
Contributor Author

@mmrazik, please review this PR.

@alexander-smolyakov
Copy link
Contributor

Hey @mmrazik, could you please review and merge this PR?

@mmrazik mmrazik merged commit 6f675a3 into microsoft:master Apr 4, 2022
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.

4 participants