-
Notifications
You must be signed in to change notification settings - Fork 25
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
base: devel
Are you sure you want to change the base?
Migrate www.transifex.com to app.transifex.com in local and root config #187
Conversation
There was a problem hiding this 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.
482ef99
to
594f02c
Compare
Hi @kbairak , thanks for the feedback. I pushed the requested update -- let me know if there is anything else. |
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:
It makes sense for the first case since we may have missed something. |
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.
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. |
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
yettx
commands continue prompting for it each time due towww.transifex.com
not being changed toapp.transifex.com
.I'll demonstrate my fix for these issues in each case below:
I.
~/.transifexrc
does not existInitial state:
Running the migration script:
Final state:
II.
~/.transifexrc
exists but it still has deprecatedwww.transifex.com
config (initial.tx/config
same as above), andwww.transifex.com
was not getting updated toapp.transifex.com
Initial state:
Running the migration script:
Final state:
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 therest_hostname
andtoken
like we expect, and also had[https://www.transifex.com]
changed to[https://api.transifex.com]
as required. As of this time, thehostname
,username
, andpassword
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 hashost = https://www.transifex.com
soGetActiveHost
always returnednil
.Initial state:
(Again, the initial
.tx/config
is the same as in the above cases)Running the migration script:
Final state:
~/.transifexrc
is unchanged from the initial state (although we created a backup anyway) and.tx/config
is properly migratedIV. 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:
www.transifex.com
to the pre-migration configs in the testwww.transifex.com
becomesapp.transifex.com
after running the migration~/.transifexrc
gets moved prior toTestNoTransifexRcFile
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~/.transifexrc
functionality oftx migrate