Project

General

Profile

Bug #15605

Feature #10034: Translation web platform

Feature #15079: Integrate the platform with our Git and ikiwiki infrastructure

Make check_po optionally accept a list of files

Added by u over 1 year ago. Updated 5 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
05/17/2018
Due date:
% Done:

100%

Feature Branch:
https://salsa.debian.org/tails-team/tails/merge_requests/14
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

Makes it more useful for users/scripts to test against only modified files and ignore the rest.

0001-Fix-typos-and-minor-improvements.patch View (1.63 KB) u, 11/02/2018 05:20 PM


Related issues

Related to Tails - Bug #15362: Pre-commit hook/Run check_po whenever we try to commit a po file in all Git repositories Resolved 03/02/2018
Related to Tails - Bug #15408: Consider forcing wrapping of po files at 79 chars per line Rejected 03/13/2018
Related to Tails - Feature #16102: List of potential checks we might want to do on PO files Resolved 11/05/2018
Blocks Tails - Bug #15403: Unify po headers Resolved 08/19/2018

Associated revisions

Revision 52afa659
Added by intrigeri 5 months ago

Merge remote-tracking branch 'origin/hefee/feature/15403-unify-po-headers'

refs: #15403, #15605

History

#1 Updated by u over 1 year ago

  • Related to Bug #15362: Pre-commit hook/Run check_po whenever we try to commit a po file in all Git repositories added

#2 Updated by u over 1 year ago

  • Related to Bug #15408: Consider forcing wrapping of po files at 79 chars per line added

#3 Updated by u over 1 year ago

#4 Updated by hefee over 1 year ago

  • % Done changed from 0 to 60

This is now possible with:
https://github.com/hefee/tails-jenkins-tools/commit/a23231e54185f371b7279d544d51368ca2932826

I needed to fork jenkins-tools as it is a submodule with a own repository.

#5 Updated by u over 1 year ago

  • Feature Branch set to jenkins-tools.git:translation_platform_hooks

I've merged this into our jenkins tools submodule and I guess we will have to ask groente to do another review and merge this into master.

#6 Updated by u over 1 year ago

  • Assignee changed from hefee to groente
  • QA Check set to Ready for QA

Hi groente, may you please review this?

Our aim here is to be able to use check_po on single files, or a list of files, instead of always running it on the entire repository. This is because we have created a git hook which will run whenever people try to commit po files and then we don't want to make their life horrible with waiting for check_po...

#7 Updated by groente over 1 year ago

  • Assignee changed from groente to hefee
  • % Done changed from 60 to 80
  • QA Check changed from Ready for QA to Pass

looks good! the only remark i have is on line 112, langauge is misspelled ;)

#8 Updated by u over 1 year ago

  • Assignee changed from hefee to u

Thanks! I'll fix this and ask intrigeri to give me his okay to merge this (test suite in mind).

#9 Updated by u over 1 year ago

  • Assignee changed from u to intrigeri
  • QA Check changed from Pass to Info Needed

The last commit we made should be at 32432bd329d9ba5b5e07e76daf87d7bd8d5ab5ed.
Can you tell me if I need to check something else before merging this? I suppose this script is run in the test suite or somewhere else on the production machine?

#11 Updated by intrigeri over 1 year ago

  • QA Check changed from Info Needed to Ready for QA

The last commit we made should be at 32432bd329d9ba5b5e07e76daf87d7bd8d5ab5ed.
Can you tell me if I need to check something else before merging this? I suppose this script is run in the test suite or somewhere else on the production machine?

OK, I'll take a look (next week).

#12 Updated by intrigeri over 1 year ago

  • Target version set to Tails_3.9

#13 Updated by intrigeri over 1 year ago

  • Target version changed from Tails_3.9 to Tails_3.8

I suppose it would smooth your work if this gets merged in time for 3.8. So I'll try to review and hopefully merge this despite the tight timing.

#14 Updated by intrigeri over 1 year ago

I'm a bit confused: the corresponding branch in tails.git updates the jenkins-tools submodule to commit 5332be2098f5564561f6f9eb274b437e97533242, but that's not the last commit on the branch in jenkins-tools.git (currently: 32432bd329d9ba5b5e07e76daf87d7bd8d5ab5ed). It would be confusing to merge these two branches as is in their respective repos, because then tails.git would reference an old version of jenkins-tools.git. I assume it's a mere oversight and if happy with the two branches, I'll bump the submodule in tails.git to its latest version.

#15 Updated by intrigeri over 1 year ago

  • Related to deleted (Bug #15403: Unify po headers)

#16 Updated by intrigeri over 1 year ago

#17 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to u
  • QA Check changed from Ready for QA to Dev Needed

I'm fine with the code changes that implement what the title of this ticket says, but the branch has other changes (new checks). I'm not sure where I should comment on these changes, between #15403 and here. Whatever, I'll do it here.

As part of the release process of our custom packages, we run check_po on PO files that come straight from Transifex. They don't satisfy all these new requirements. So either the "Upgrade bundled binary Debian packages" section of wiki/src/contribute/release_process.mdwn must be adjusted to use your code that unifies PO headers, or there must be a way to opt-out of these new checks.

Otherwise, code review passes. I've pushed a trivial typo fix on top of your branch.

#18 Updated by intrigeri over 1 year ago

  • Target version changed from Tails_3.8 to Tails_3.9

#19 Updated by u over 1 year ago

intrigeri wrote:

I'm fine with the code changes that implement what the title of this ticket says, but the branch has other changes (new checks). I'm not sure where I should comment on these changes, between #15403 and here. Whatever, I'll do it here.

Ack, thanks.

As part of the release process of our custom packages, we run check_po on PO files that come straight from Transifex. They don't satisfy all these new requirements. So either the "Upgrade bundled binary Debian packages" section of wiki/src/contribute/release_process.mdwn must be adjusted to use your code that unifies PO headers, or there must be a way to opt-out of these new checks.

Right. There is currently no way to opt out and we have to think about it. Oversight.

Otherwise, code review passes. I've pushed a trivial typo fix on top of your branch.

Thanks, I'll have a look.

#20 Updated by u about 1 year ago

  • Target version changed from Tails_3.9 to Tails_3.10.1

Won't be able to review this in the next 48 hours (release of 3.9)

#21 Updated by intrigeri about 1 year ago

  • Target version changed from Tails_3.10.1 to Tails_3.11

#22 Updated by u about 1 year ago

  • Assignee changed from u to hefee

#23 Updated by u about 1 year ago

intrigeri wrote:

Otherwise, code review passes. I've pushed a trivial typo fix on top of your branch.

I'm not able update the submodule in the branch (completely able 'git submodule update --init' which seems to fetch from whereever, but not the branch I've currently checked out) . Any idea how to do this properly?

#24 Updated by u about 1 year ago

Because I cannot push, I'll simply add a patch here. Last commit before the patch in this branch was 32432bd329d9ba5b5e07e76daf87d7bd8d5ab5ed

#25 Updated by intrigeri about 1 year ago

(I'm not sure if you're asking me. In doubt, happy to help with submodules stuff :)

I don't understand what "completely able 'git submodule update --init' which seems to fetch from whereever, but not the branch I've currently checked out" means.

FWIW, I can fetch the submodule just fine and the state of the branch in jenkins-tools.git seems to be as intended:

$ cd submodules/jenkins-tools/ && git fetch
$ git remote -v
origin    tails@git.tails.boum.org:jenkins-tools (fetch)
origin    tails@git.tails.boum.org:jenkins-tools (push)
$ git log -1 --pretty=oneline origin/translation_platform_hooks
44520f80b8dce9657dfeaae36650a3725ce82d1c (origin/translation_platform_hooks) Fix typo.

Does this produce a different output for you?

Now, the branch in tails.git references an old version of the submodule:

$ cd /my/tails/git/checkout && git checkout translation_platform_hooks && git reset --hard @{u} && git submodule update --init
$ cd submodules/jenkins-tools
$ git log --pretty=oneline ..origin/translation_platform_hooks
44520f80b8dce9657dfeaae36650a3725ce82d1c (origin/translation_platform_hooks) Fix typo.
32432bd329d9ba5b5e07e76daf87d7bd8d5ab5ed Merge remote-tracking branch 'hefee/translation_platform_hooks' into translation_platform_hooks
2fc2faa5c5335046cb3162eddd03819d3b8634e0 check for 79 chars.
a9e1565c0a752541af3fc2b3b71b624c4c9fb3e1 Minor spacing or comment improvements

So presumably you need to do something like this:

$ cd /my/tails/git/checkout && git checkout translation_platform_hooks && git pull && git submodule update --init
$ cd submodules/jenkins-tools
$ git checkout translation_platform_hooks
$ git merge origin/translation_platform_hooks
# here you apply your additional commit
$ git push -u origin translation_platform_hooks
$ cd ../../
$ git commit -m "Update the jenkins-tools submodule." submodules/jenkins-tools
$ git push origin translation_platform_hooks

Happy to help if that's not enough, once I understand what exact problem you're facing.

#26 Updated by hefee about 1 year ago

  • Related to Feature #16102: List of potential checks we might want to do on PO files added

#27 Updated by hefee about 1 year ago

  • Status changed from In Progress to 11
  • % Done changed from 80 to 100

The original bug "accept a list of files" for check_po is solved.
About @u issue of not able to push - please move this discussion to another place.

#28 Updated by CyrilBrulebois 11 months ago

  • Status changed from 11 to Resolved

#29 Updated by hefee 11 months ago

  • Status changed from Resolved to In Progress
  • Target version changed from Tails_3.11 to Tails_3.12
  • QA Check changed from Dev Needed to Ready for QA

I used Fix commited as status, as it is commit on a local branch, that will together with other task review together. I use now the more general "In Progress" for the moment.

#30 Updated by intrigeri 11 months ago

I used Fix commited as status, as it is commit on a local branch, that will together with other task review together.

In general "Fix committed" means "reviewed and merged somewhere that will for sure be released/deployed". So "In progress" seems indeed closer to what you mean :)

#31 Updated by anonym 10 months ago

  • Target version changed from Tails_3.12 to Tails_3.13

#32 Updated by u 10 months ago

  • Parent task changed from #10034 to #15079

#33 Updated by hefee 10 months ago

  • Description updated (diff)
  • Status changed from In Progress to Resolved

#34 Updated by hefee 10 months ago

  • Subject changed from Make check_po.sh optionally accept a list of files to Make check_po optionally accept a list of files

Maked it as resolved, as the new check_po has this feature implemented (not reviewed yet).

#35 Updated by hefee 10 months ago

  • Status changed from Resolved to In Progress

#36 Updated by u 9 months ago

I see you reopened it: good. "Resolved" means: reviewed & merged.
I'm not sure who will review this one. I would propose that this be assigned to intrigeri for review and merging.

#37 Updated by intrigeri 8 months ago

  • Assignee changed from hefee to intrigeri
  • Target version changed from Tails_3.13 to Tails_3.15

Will review this at the sprint.

#38 Updated by intrigeri 6 months ago

  • Assignee changed from intrigeri to hefee
  • QA Check changed from Ready for QA to Info Needed
  • Feature Branch deleted (jenkins-tools.git:translation_platform_hooks)

@hefee, could you please point the "Feature Branch" field to wherever the code I should review lives? (jenkins-tools.git:translation_platform_hooks was last modified almost a year ago, and does much more than what this ticket is about, so I assume that's not it.)

#39 Updated by intrigeri 6 months ago

  • QA Check deleted (Info Needed)

#40 Updated by hefee 6 months ago

  • Assignee changed from hefee to intrigeri
  • Feature Branch set to https://salsa.debian.org/tails-team/tails/merge_requests/14

As the complete check_po got rewritten, this is now done with unify_po-headers.py (see #15403).

#41 Updated by intrigeri 6 months ago

  • Status changed from In Progress to Needs Validation

Thanks!

#42 Updated by intrigeri 5 months ago

  • Status changed from Needs Validation to In Progress
  • Assignee changed from intrigeri to hefee

hefee wrote:

As the complete check_po got rewritten, this is now done with unify_po-headers.py (see #15403).

@hefee, then I don't understand what I'm supposed to review this (#15605) very ticket. I see there's an ongoing review process for !14 by enrico and I'd rather not start another concurrently one. Could you please clarify what you're expecting from me here? Thanks in advance!

#43 Updated by hefee 5 months ago

intrigeri wrote:

@hefee, then I don't understand what I'm supposed to review this (#15605) very ticket. I see there's an ongoing review process for !14 by enrico and I'd rather not start another concurrently one. Could you please clarify what you're expecting from me here? Thanks in advance!

enrico reviews only the security impact of the script and not the features. So your review step would be to check if the new script is able to handle a list of files. But yeah you can and should comment on !14 and not start a second one.

#44 Updated by intrigeri 5 months ago

  • Status changed from In Progress to Needs Validation
  • Assignee changed from hefee to intrigeri

So your review step would be to check if the new script is able to handle a list of files. But yeah you can and should comment on !14 and not start a second one.

Thanks! :)

#45 Updated by intrigeri 5 months ago

  • Status changed from Needs Validation to In Progress
  • Assignee changed from intrigeri to hefee

See the Salsa MR :)

#46 Updated by intrigeri 5 months ago

  • Status changed from In Progress to Resolved

#47 Updated by intrigeri 5 months ago

  • Assignee deleted (hefee)

Also available in: Atom PDF