[PM-16533] Rename files to indicate they belong to Password Manager #1231
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1231 +/- ##
=======================================
Coverage 89.43% 89.43%
=======================================
Files 725 725
Lines 45821 45821
=======================================
Hits 40979 40979
Misses 4842 4842 ☔ View full report in Codecov by Sentry. |
|
Great job, no security vulnerabilities found in this Pull Request |
KatherineInCode
requested review from
a team,
matt-livefront,
phil-livefront and
brant-livefront
as code owners
.github/workflows/build.yml
Outdated
| yq -i '.settings.MARKETING_VERSION = "${{ steps.version_info.outputs.version_name }}"' 'project_passwordmanager.yml' | ||
| yq -i '.settings.CURRENT_PROJECT_VERSION = "${{ steps.version_info.outputs.version_number }}"' 'project_passwordmanager.yml' |
There was a problem hiding this comment.
🤔 As far as I can see we're using hyphen - instead of underscore _ to separate words in the filenames for yml like crowdin-pull.yml or github-release.yml. Is there a reason to change to underscore for the new ones in this PR?
🤔 Also what do you think of using the abbreviate version of Password Manager => PM? So then we can use BWA for the authenticator.
There was a problem hiding this comment.
Both of those seem very reasonable to me; when I wrote the PM section of the Action Plan I was patterning off of what @vvolkgang had done for the BWA section, but I think hyphens and abbreviations is a good way to go. Álison, what do you think?
If we do this with hyphens and abbreviations, I can get the Action Plan updated to match.
There was a problem hiding this comment.
That just happened to be the original name of .license_plist.yml. We can rename them all to use - while at it.
Let's go with the shorter names. We'll scrap some / all instances of these later while merging files.
There was a problem hiding this comment.
Awesome, I'll get the Action Plan updated accordingly, and then make the change here
There was a problem hiding this comment.
🤔 Should this file also be renamed to use hyphen as well?
There was a problem hiding this comment.
Hm. I was going off of all of the scripts (except download-artifacts.sh) using underscores. I'm not sure if updating all of them to also be hyphens would be in the scope of this change, though? If we want to do that?
(I think it's reasonable for us to do that at this point, to match everything else; it's more a question of whether this is the best place for that or not)
There was a problem hiding this comment.
I'm fine to leave that for another PR if you feel it's out of the scope of this PR, no problem at all. Approving as is 👍
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-16533
📔 Objective
As part of the plan to merge the Authenticator repository into this one, we'll want to avoid filename conflicts. This moves various YML files out of the way by specifying that they're for Password Manager, so that the Authenticator ones will be able to sit next to them fine once the merge is done.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes