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

[ADD] get pins script #26

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

gfcapalbo
Copy link
Contributor

@gfcapalbo gfcapalbo commented Apr 6, 2023

Script to extract all pins from a built waft instance.
use:
to replicate faithfully the pins of a test instance on a prd instance.

@gfcapalbo gfcapalbo marked this pull request as draft April 6, 2023 15:16
@gfcapalbo gfcapalbo force-pushed the master-get_pins_script branch from 6014517 to 7176c2c Compare April 12, 2023 13:26
@gfcapalbo gfcapalbo marked this pull request as ready for review April 12, 2023 14:17
@gfcapalbo gfcapalbo force-pushed the master-get_pins_script branch 2 times, most recently from 98c11e1 to 7f1383d Compare April 12, 2023 14:21
@gfcapalbo
Copy link
Contributor Author

@thomaspaulb @Hussam-Suleiman completed, ready for review.

./get_pins will output the text of a pinned repos.yaml of the current deployment.
Very useful when updating production from test, we can extract the pins of current test instance quickly, and copy them to our production repos.yaml

The script pins everything, here is an example of running it on my local FF test
(attached repos.yaml and output) (diff screenshot)

Diff

there is also an option --only-base=True where it pins only the first merge (base branch).
By default it will pin all MR's.
I checked 4-5 MR's and the pin was correct.

I also built with the output of the script and all worked well.

PROPOSED EXTENSION:

  • calculate the needed minimum depth, and modify that too , in case the youngest common ancestor is now beyond depth.
  • suggest a modification to addons.yaml if it does not contain all repos in repos.yaml

As it is now , output can be copy/pasted in repos.yaml and it works.

@gfcapalbo
Copy link
Contributor Author

@thomaspaulb i put this in WIP again.
I need to support when "target:" is used.
we usually don;'t use it , because jan made a MR where target is not mandatory anymore (the first merge is the base branch) / but i saw a couple of waft instances where it is used so i want to add support/

@gfcapalbo
Copy link
Contributor Author

gfcapalbo commented Apr 19, 2023

@thomaspaulb Refactor done and added support for pinning target:

example:
. get_pins --only-base=True

gives:


`OCA/bank-payment:
  defaults:
    depth: 2000
  remotes:
    origin: https://github.com/OCA/bank-payment.git
    wpichler: https://github.com/wpichler/bank-payment.git
    acsone: https://github.com/acsone/bank-payment.git
  target: origin ${ODOO_VERSION} 69083802dfcfa20ea8ed1a6f94b6abe868c32c5c
  merges:
  - origin ${ODOO_VERSION}
  - wpichler 16.0-mig-account_payment_order
  - acsone 16.0-mig-account_payment_mode
`

While . get_pins gives:

`OCA/bank-payment:
  defaults:
    depth: 2000
  remotes:
    origin: https://github.com/OCA/bank-payment.git
    wpichler: https://github.com/wpichler/bank-payment.git
    acsone: https://github.com/acsone/bank-payment.git
  target: origin ${ODOO_VERSION} 69083802dfcfa20ea8ed1a6f94b6abe868c32c5c
  merges:
  - origin ${ODOO_VERSION} 69083802dfcfa20ea8ed1a6f94b6abe868c32c5c
  - wpichler 16.0-mig-account_payment_order 6e7bfc43ac767c4bfef4ce4bd5bb5e614fec3050
  - acsone 16.0-mig-account_payment_mode de70eee76f47c18e72585520ad43d91f0ba2ee13
`

bin/get_pins Outdated Show resolved Hide resolved
@NL66278
Copy link
Contributor

NL66278 commented Apr 28, 2023

@gfcapalbo Please use:

if __name__ == "__main__":
    sys.exit(main())

And a Usage function as described here: https://www.artima.com/weblogs/viewpost.jsp?thread=4829

(But keep the more modern argparse etc. the blog was written a long time ago).

@gfcapalbo
Copy link
Contributor Author

gfcapalbo commented May 4, 2023

@NL66278 @thomaspaulb added support for imposing the correct merge, and followed through @NL66278 's comments/

Diff, you can see that where the necessary depth merge exceeds the waft_depth_merge variable it is replaced by the correct value + 25. ( I added 25 because if we used the precise value, by the time we deploy the new yaml , new commits may have already been added, making that value insufficient)

image

So now the script offers complete pinning and the certainty of "sufficient" depth values for every repo.

bin/get_pins Outdated Show resolved Hide resolved
bin/get_pins Outdated Show resolved Hide resolved
bin/get_pins Outdated Show resolved Hide resolved
bin/get_pins Outdated Show resolved Hide resolved
Copy link
Member

@thomaspaulb thomaspaulb left a comment

Choose a reason for hiding this comment

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

Review

@gfcapalbo
Copy link
Contributor Author

@thomaspaulb

could not reply directly in threads.... some bug.
Added verbosity:

......now it notifies:
==> processing repo R
===> processing merge X
Updating pin, inserting new pin, increasing depth merge from xx to xy

Also replacing all ENV variables as suggested, that means all WAFT_DEPTH_MERGE and all WAFT_DEPTH_DEFAULT are replaced too, is that okay?

Screenshot from 2023-05-09 13-47-30

@gfcapalbo
Copy link
Contributor Author

@thomaspaulb ALl requests done. I will make Danny's translation script execution in a separate branch.

@gfcapalbo
Copy link
Contributor Author

@thomaspaulb it seems that danny has alreadyu done the script-calling-python thing in MR:
https://github.com/sunflowerit/waftlib/pull/25/files#diff-9a11c011d63d766654c62553da4f7b447d1aace37001ccde186193f26f2c7a15

merged.

Copy link
Member

@thomaspaulb thomaspaulb left a comment

Choose a reason for hiding this comment

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

Some comments still

get_pins Show resolved Hide resolved
bin/get_pins.py Show resolved Hide resolved
bin/get_pins.py Show resolved Hide resolved
@gfcapalbo gfcapalbo force-pushed the master-get_pins_script branch from f1f1e4a to 47aeef2 Compare May 19, 2023 13:12
@gfcapalbo
Copy link
Contributor Author

gfcapalbo commented May 19, 2023

@thomaspaulb done all suggestions:

 - Made executable and updated Readme
 - Made variable substitution only per-line when needed , no information lost when no subst needed 
 - rebased on more current master.
 - tested in 2 different wafts.
 - ran black.
 - rebased in 2 commits , so that latest changes are in separate commit.
 - will rebase in 1 commit when approved.

@gfcapalbo gfcapalbo force-pushed the master-get_pins_script branch from 5d037bf to eadeb3d Compare May 19, 2023 14:21
@gfcapalbo
Copy link
Contributor Author

@thomaspaulb tested for more than an Hour on FF prd and FF tst, the int > string comparison error never happens and never can happen after my latest update. please verify latest version installed.

@gfcapalbo gfcapalbo force-pushed the master-get_pins_script branch from eadeb3d to c10144d Compare September 7, 2023 14:23
@gfcapalbo gfcapalbo force-pushed the master-get_pins_script branch from c10144d to 106322a Compare September 7, 2023 14:23
@ddejong-therp
Copy link
Contributor

Hey giovanni, maybe it would be nice if the script has the execution permission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants