Skip to content

Conversation

@rebeccaalpert
Copy link
Contributor

@rebeccaalpert rebeccaalpert commented Apr 28, 2021

Added one script to handle project creation, file generation, and file upload for Memsource and a second to handle download and file import into OpenShift.

I used https://github.com/unofficial-memsource/memsource-cli-client instead of the Ansible collection because I couldn't get the collection to work. We can always circle back and switch to that later.

Documentation is in the files themselves; installing the memsource cli client and providing credentials is required for use. One script also requires the rename utility.

I also made some revisions to the other scripts in our little tool chain here.

Fixes https://issues.redhat.com/projects/CONSOLE/issues/CONSOLE-2508.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2021
@rebeccaalpert
Copy link
Contributor Author

Updated PR to add the download script. Let me know if you have feedback. I also addressed the feedback on script #1.

@rebeccaalpert rebeccaalpert changed the title [WIP] Memsource automation CONSOLE-2508: Memsource automation Apr 29, 2021
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2021
@rebeccaalpert rebeccaalpert force-pushed the memsource-automation branch 2 times, most recently from ace9179 to bd1356c Compare April 29, 2021 16:29
@rebeccaalpert
Copy link
Contributor Author

Just realized I didn't handle creating a new commit and PR in the script. I'll see if I can add that in.

@rebeccaalpert rebeccaalpert changed the title CONSOLE-2508: Memsource automation [WIP] CONSOLE-2508: Memsource automation Apr 29, 2021
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dev-console Related to dev-console component/gitops Related to gitops-plugin component/helm Related to helm-plugin component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/lso Related to local-storage-operator-plugin component/metal3 Related to metal3-plugin component/network-attachment-definition Related to network-attachment-definition component/olm Related to OLM component/pipelines Related to pipelines-plugin component/shared Related to console-shared component/topology Related to topology labels Apr 29, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dtaylor113, rebeccaalpert
To complete the pull request process, please assign jhadvig after the PR has been reviewed.
You can assign the PR to them by writing /assign @jhadvig in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dtaylor113
Copy link
Contributor

Nice job @rebeccaalpert! Nice bit of scripting! :-)

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

@rebeccaalpert A lot of variables are still missing quotes. Can you run shellcheck on the scripts and fix any errors? Thanks!

@openshift-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2021
@rebeccaalpert
Copy link
Contributor Author

Fixed the stuff shellcheck called out.

@spadgett
Copy link
Member

spadgett commented May 3, 2021

Hey, @rebeccaalpert. There still seem to be warnings. Can you take another look? (Don't worry about the "not following" warnings.)

❯ shellcheck frontend/i18n-scripts/*.sh

In frontend/i18n-scripts/export-pos.sh line 5:
source ./i18n-scripts/languages.sh
       ^-------------------------^ SC1091: Not following: ./i18n-scripts/languages.sh was not specified as input (see shellcheck -x).


In frontend/i18n-scripts/export-pos.sh line 10:
  yarn i18n-to-po -f "$(basename "$f" .json)" -l $i
                                                 ^-- SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
  yarn i18n-to-po -f "$(basename "$f" .json)" -l "$i"


In frontend/i18n-scripts/export-pos.sh line 18:
    for f in $d/locales/en/* ; do
             ^-- SC2231: Quote expansions in this for loop glob to prevent wordsplitting, e.g. "$dir"/*.txt .


In frontend/i18n-scripts/export-pos.sh line 21:
        yarn i18n-to-po -p "$d" -f "$(basename "$f" .json)" -l $i
                                                               ^-- SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
        yarn i18n-to-po -p "$d" -f "$(basename "$f" .json)" -l "$i"


In frontend/i18n-scripts/languages.sh line 1:
LANGUAGES=( 'ja' 'zh-cn' 'ko')
^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
^-------^ SC2034: LANGUAGES appears unused. Verify use (or export if used externally).


In frontend/i18n-scripts/memsource-download.sh line 5:
source ./i18n-scripts/languages.sh
       ^-------------------------^ SC1091: Not following: ./i18n-scripts/languages.sh was not specified as input (see shellcheck -x).


In frontend/i18n-scripts/memsource-upload.sh line 5:
source ./i18n-scripts/languages.sh
       ^-------------------------^ SC1091: Not following: ./i18n-scripts/languages.sh was not specified as input (see shellcheck -x).

For more information:
  https://www.shellcheck.net/wiki/SC2148 -- Tips depend on target shell and y...
  https://www.shellcheck.net/wiki/SC2034 -- LANGUAGES appears unused. Verify ...
  https://www.shellcheck.net/wiki/SC1091 -- Not following: ./i18n-scripts/lan...

@rebeccaalpert
Copy link
Contributor Author

rebeccaalpert commented May 3, 2021

@spadgett - I'm not getting those anyway:

(.memsource) MacBook-Pro:frontend ralpert$ shellcheck -x i18n-scripts/memsource-download.sh
(.memsource) MacBook-Pro:frontend ralpert$ shellcheck -x i18n-scripts/memsource-upload.sh

@rebeccaalpert rebeccaalpert force-pushed the memsource-automation branch from c24bb9c to 2f167f0 Compare May 3, 2021 21:45
Comment on lines 43 to 45
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I think it would be better to just avoid the special characters in the filename altogether, particular if Memsource does weird things with them. Then we can also avoid needing the rename utility. Would a convention like my-package__my-namespace be simpler? (Similar to BEM names.)

Copy link
Contributor Author

@rebeccaalpert rebeccaalpert May 5, 2021

Choose a reason for hiding this comment

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

I can make the change, but I'd have to rework two other scripts and we won't be able to download any older files with this system.

Copy link
Member

Choose a reason for hiding this comment

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

I can make the change, but I'd have to rework two other scripts.

I think it's worth doing.

@rebeccaalpert
Copy link
Contributor Author

Addressed feedback. I adjusted naming scheme in a separate commit so we can drop it if needed. Note that this change will prevent us from using any of the scripts for dealing with translation files that we've already sent over under the original naming scheme (we have one batch currently in process).

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

@rebeccaalpert rebeccaalpert force-pushed the memsource-automation branch from 1bd3474 to 015568b Compare May 6, 2021 13:50
@rebeccaalpert
Copy link
Contributor Author

All set.

@rebeccaalpert rebeccaalpert requested a review from spadgett May 10, 2021 20:09
Added scripts to handle project creation, file generation, and file upload for Memsource, as well as file download and import.

Fixes https://issues.redhat.com/projects/CONSOLE/issues/CONSOLE-2508
@rebeccaalpert rebeccaalpert force-pushed the memsource-automation branch from 015568b to fa35b9d Compare May 10, 2021 20:53
@rebeccaalpert
Copy link
Contributor Author

@spadgett; I squashed; let me know if this is good to go or if you have additional feedback. I used it to upload Sprint 200, but Sprint 199 will be in the old format and we'll have to revert the tools to the old naming scheme locally to import those.

Copy link
Member

@spadgett spadgett 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 @rebeccaalpert.

@spadgett
Copy link
Member

/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 12, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 12, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtaylor113, rebeccaalpert, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2021
@openshift-merge-robot openshift-merge-robot merged commit af5ebed into openshift:master May 13, 2021
@spadgett spadgett added this to the v4.8 milestone May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants