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

Migrate www.transifex.com to app.transifex.com in local and root config #187

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

wbclark
Copy link

@wbclark wbclark commented Apr 27, 2023

This commit handles various flaws with tx migrate and improves the tests for the migration script. For example, it was previously possible to provide the API token and have it saved to ~/.transifexrc yet tx commands continue prompting for it each time due to www.transifex.com not being changed to app.transifex.com.

I'll demonstrate my fix for these issues in each case below:

I. ~/.transifexrc does not exist

Initial state:

[wclark@centos8-stream-wclark-katello-devel katello]$ cat .tx/config
[main]
host = https://www.transifex.com

[foreman.katello]
file_filter = locale/<lang>/katello.edit.po
source_file = locale/katello.pot
source_lang = en
type = PO

[foreman.bastion_katello]
file_filter = engines/bastion_katello/app/assets/javascripts/bastion_katello/i18n/locale/<lang>.po
source_file = engines/bastion_katello/app/assets/javascripts/bastion_katello/i18n/bastion_katello.pot
source_lang = en
type = PO

[wclark@centos8-stream-wclark-katello-devel katello]$ ll ~/.transifexrc
ls: cannot access '/home/wclark/.transifexrc': No such file or directory

Running the migration script:

[wclark@centos8-stream-wclark-katello-devel katello]$ ../transifex-cli/bin/tx migrate
Root configuration file not found -- creating it at `/home/wclark/.transifexrc`.
Please provide an API token to continue.
If you don't have an API token, you can generate one in https://app.transifex.com/user/settings/api/
> ***REDACTED_API_TOKEN***
No rest_hostname found. Adding `rest.api.transifex.com`
Migration ended! We have created a backup file for your previous /home/wclark/katello/.tx/config at `/home/wclark/katello/.tx/config_20230427063500.bak`.
Additionally, we have created a new /home/wclark/.transifexrc with your token.

Final state:

[wclark@centos8-stream-wclark-katello-devel katello]$ cat .tx/config
[main]
host = https://app.transifex.com

[o:foreman:p:foreman:r:bastion_katello]
file_filter = engines/bastion_katello/app/assets/javascripts/bastion_katello/i18n/locale/<lang>.po
source_file = engines/bastion_katello/app/assets/javascripts/bastion_katello/i18n/bastion_katello.pot
source_lang = en
type        = PO

[o:foreman:p:foreman:r:katello]
file_filter = locale/<lang>/katello.edit.po
source_file = locale/katello.pot
source_lang = en
type        = PO

[wclark@centos8-stream-wclark-katello-devel katello]$ cat ~/.transifexrc
[https://app.transifex.com]
rest_hostname = https://rest.api.transifex.com
token         = ***REDACTED_API_TOKEN***

II. ~/.transifexrc exists but it still has deprecated www.transifex.com config (initial .tx/config same as above), and www.transifex.com was not getting updated to app.transifex.com

Initial state:

[wclark@centos8-stream-wclark-katello-devel katello]$ cat ~/.transifexrc
[https://www.transifex.com]
hostname      = https://www.transifex.com
username      = api
password      = ***REDACTED_API_TOKEN***

Running the migration script:

[wclark@centos8-stream-wclark-katello-devel katello]$ ../transifex-cli/bin/tx migrate
Found API token in `/home/wclark/.transifexrc` file
No rest_hostname found. Adding `rest.api.transifex.com`
Migration ended! We have created a backup file for your previous /home/wclark/katello/.tx/config at `/home/wclark/katello/.tx/config_20230427065348.bak`.
Additionally, we have created a backup file for your previous /home/wclark/.transifexrc at `/home/wclark/.transifexrc_20230427065348.bak`.

Final state:

[wclark@centos8-stream-wclark-katello-devel katello]$ cat ~/.transifexrc
[https://app.transifex.com]
hostname      = https://www.transifex.com
username      = api
password      = ***REDACTED_API_TOKEN***
rest_hostname = https://rest.api.transifex.com
token         = ***REDACTED_API_TOKEN***

In this case, the .tx/config is migrated properly as expected (I didn't include the output, it's the same as above).

NOTE: ~/.transifexrc has the addition of the rest_hostname and token like we expect, and also had [https://www.transifex.com] changed to [https://api.transifex.com] as required. As of this time, the hostname, username, and password are unchanged (but we could remove these as well, as they are no longer needed, and we created a backup of the original ~/.transifexrc file anyway)

III. We also now properly handle the case where ~/.transifexrc has already been fully updated to use the new configuration (i.e. the user already updated their config when working on a different repository) but the local .tx/config still needs migration... previously this would also continue prompting for API token each time because local config still has host = https://www.transifex.com so GetActiveHost always returned nil.

Initial state:

[wclark@centos8-stream-wclark-katello-devel katello]$ cat ~/.transifexrc
[https://app.transifex.com]
rest_hostname = https://rest.api.transifex.com
token         = ***REDACTED_API_TOKEN***

(Again, the initial .tx/config is the same as in the above cases)

Running the migration script:

[wclark@centos8-stream-wclark-katello-devel katello]$ ../transifex-cli/bin/tx migrate
Migration ended! We have created a backup file for your previous /home/wclark/katello/.tx/config at `/home/wclark/katello/.tx/config_20230427070639.bak`.
Additionally, we have created a backup file for your previous /home/wclark/.transifexrc at `/home/wclark/.transifexrc_20230427070639.bak`.

Final state:

~/.transifexrc is unchanged from the initial state (although we created a backup anyway) and .tx/config is properly migrated

IV. In addition to fixing the migration script to better handle all of the above cases, I also improved the tests for the migration script in the following ways:

  1. Restore www.transifex.com to the pre-migration configs in the test
  2. Test that www.transifex.com becomes app.transifex.com after running the migration
  3. Ensure that when running tests locally, the developer's ~/.transifexrc gets moved prior to TestNoTransifexRcFile so that it doesn't interfere with the test; following the test, the newly created ~/.transifexrc gets deleted and the developer's original ~/.transifexrc gets restored
  4. Added additional assertions to test the backup ~/.transifexrc functionality of tx migrate

Copy link
Member

@kbairak kbairak left a comment

Choose a reason for hiding this comment

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

Hey @wbclark, Thank you very much for the contribution.

For now I only have this one minor comment. I am still reviewing the PR as a whole. Since there are a lot of combinations for initial states, I need some time go get my head around it. I will keep you updated.

Thanks again.

cmd/tx/main.go Outdated Show resolved Hide resolved
@wbclark wbclark force-pushed the main_host_migrate_app.transifex.com branch from 482ef99 to 594f02c Compare May 2, 2023 12:10
@wbclark
Copy link
Author

wbclark commented May 2, 2023

Hi @kbairak , thanks for the feedback.

I pushed the requested update -- let me know if there is anything else.

@codegaze
Copy link
Member

codegaze commented May 2, 2023

Hello, @wbclark. We appreciate the contribution! 🎉

Just have a few questions to understand a bit better about the issue, mostly out of curiosity since the changes look legit:

  • Was this issue introduced when we applied the app.transifex.com change?
  • Do the changes assume that the migration script was executed twice?

It makes sense for the first case since we may have missed something.
For the last two cases, if we assume that the script was run once to migrate from the old CLI, we shouldn't fall into these cases, right?

@wbclark
Copy link
Author

wbclark commented May 2, 2023

Was this issue introduced when we applied the app.transifex.com change?

Hi @codegaze , I think this is exactly right.

a42522c changed the initial state in all the migration tests to use app.transifex.com without any test that the migration applies this change, or change in the migration script to apply the change. My change is basically the completion for a42522c such that both local and root config get updated to use app.transifex.com regardless of the starting scenario.

As a result, if you are in my situation -- a first time release owner for a project which uses transifex -- then you create your root config with the correct app.transifex.com but the project config still has www.transifex.com and this never changes. As a result, every time you execute a command, you get prompted for the API token again, even after running the migration, because it's never able to match the local config to the root config w/ the saved token.

My commit fixes this issue, and related issues for other initial configurations.

Do the changes assume that the migration script was executed twice?

For users that were not able to tx-pull because it tried to connect to the old www.transifex.com endpoint, or for users that got prompted for their API key every time even after entering it and having it saved in their root config file, then running the script again with my patch fixes the issue.

For users that didn't yet migrate at all, running the migration once with my patch will get them to the correct configuration the first time. It won't be necessary to run it again.

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.

None yet

3 participants