What are the suggestions here to consider incorporating? First from a slack thread: I am adding @Karim Baaba and @Wanting Shao because they were in the intial ideation regarding the inheritance / subbrands, I am currently reviewing but could you guys give a :eyes: too please? (edited)
Will Larson
Today at 8:39 AM
100% please do
Will Larson
Today at 8:40 AM
(this PR also reveals a lot of differences between crate in stg and prd, and zero between cb2 stg and prd, which I think is probably a real issue in terms of it being very easy to miss bring forward stuff to prd in our historical main vs release and also having 2 independent files)
Will Larson
Today at 8:40 AM
(but that's not specific to the inherit stuff, just stuff for us to debug independently)
Adam Kim
Today at 8:41 AM
yeah I am not surprised, it's more the "human attention to detail" is what we are fighting for issue
Will Larson
Today at 8:41 AM
+1, I think with this plus the changes that @Xingtan Hu helped with yesterday to move off cherry-picking, we will finally be in a place where it's easy to get right, versus easy to get wrong
Adam Kim
Today at 8:51 AM
Other than the inheritance logic, it's interesting that you picked rewards.json for this.
:thinking_face: and I don't have an answer to this so I am just going to provide a context (what I know).
Currently an offer (which is what the rewards.json is coming from) is created via retool app and requires humans to copy and paste the offer configuration uuid.
There are 2 distinct set of copy in this:
Offer's innate copy that is set in online system
Offer's rewards.json copy that is set in the asset repo
I was originally thinking that we might actually pull this rewards.json creation into product config but obviously that's up for discussion (edited)
Will Larson
Today at 9:00 AM
@adam -- in the fullness of time, I agree that we do want all of this in product config. My understand of the problem to be solved today is finding a intermediate improvement that allows us to reduce the maintenance overhead of having e.g. 22 copies of these rewards files that are very hard to maintain accurately. Do you think I'm reasoning about this the wrong way?
Adam Kim
Today at 9:01 AM
Yeah I think this is a good addition to that. I am just providing context and not dissenting to the PR
Will Larson
Today at 9:02 AM
sg, fwiw I actually want to eliminate almost all of these repos entirely and get down to almost exclusively product config as we get to a place where customers can manage their own config
Adam Kim
Today at 9:02 AM
fwiw that's my goal too given no constraints
Wanting Shao
Today at 1:35 PM
:wow-amaze: This script is a huge help for maintaining the future 22 sub brand products rewards.json. but I assume engineers still need to manually run this script locally and submit the PR with changes in 22 folders and have it reviewed, correct?
Karim Baaba
37 minutes ago
having a look :eyes:
Karim Baaba
26 minutes ago
@Will Larson I left a few comments. Overall looks like a good improvement!
Karim Baaba
24 minutes ago
I'm still unsure what we want to do with this setup long term—the concept of static JSON files as configs uploaded directly to S3 is pretty fragile.
Will Larson
15 minutes ago
@Wanting Shao yeah, they would need to run it, although the build will fail if they don't run it, so at least they won't introduce errors by forgetting. I agree with @Karim Baaba that long-term this isn't really right solution, but I am hopeful we can at least get into a place where this is easier to maintain before we are able to move everything into app configuration (<-- where I think @adam / @Joy Ebertz / I all want things to go long term)
Wanting Shao
9 minutes ago
:thinking_face: I think we want to find an approach to maintain the shared value props in one single file instead of 22, maybe introduce an additional api call on FE
Will Larson
5 minutes ago
we could move the compiling step to be part of a github action instead, so engineers don't even need to check in the compiled components
Will Larson
4 minutes ago
probably not as v1, want to get small version merged that just generates same static files, but once we get that working, it's easy enough to stop checking in the generated files
Joy Ebertz
1 minute ago
one thing I was playing with when I was doing this elsewhere was making something like this as a commit hook, which might work here too?
Will Larson
Just now
@Joy Ebertz -- +1, good suggestion, let me add that to this PR as part of other fixes and then from PR feedback: ```.github/workflows/inherit-validate.yml
id: changed-files
run: |
# Check if inherit files or managed content files changed
if git diff --name-only origin/${{ github.base_ref }}...HEAD | grep -E '(^inherit/|content/./PRDT-v1-30763e2e-7278-43ca-82b0-cbf6cf7351bb/|content/./PRDT-v1-7d08e2bc-d79a-4473-97f6-59c34b9ecfd9/)'; then
@karim-imprint
karim-imprint
46 minutes ago
Is there harm in running this all the time? (no check for changed files)
.github/workflows/inherit-validate.yml
name: Validate Inheritance Files
on:
pull_request:
@karim-imprint
karim-imprint
45 minutes ago
We should run this pre-deploy as well
@wrl-ip Reply...
scripts/validate_inherit.py
print()
print("To fix, either:")
print(" 1. Update inherit/ files to match desired content")
print(" 2. Run 'python scripts/expand_inherit.py' to regenerate content/")
@karim-imprint
karim-imprint
39 minutes ago
Suggested change
print(" 2. Run 'python scripts/expand_inherit.py' to regenerate content/")
print(" 2. Run 'python3 scripts/expand_inherit.py' to regenerate content/")``` .github/workflows/inherit-validate.yml
id: changed-files
run: |
# Check if inherit files or managed content files changed
if git diff --name-only origin/${{ github.base_ref }}...HEAD | grep -E '(^inherit/|content/./PRDT-v1-30763e2e-7278-43ca-82b0-cbf6cf7351bb/|content/./PRDT-v1-7d08e2bc-d79a-4473-97f6-59c34b9ecfd9/)'; then
Author
@wrl-ip
wrl-ip
6 hours ago
😢 will work to make this less incredible brittle after collecting other feedback
@karim-imprint
karim-imprint
46 minutes ago
Is there harm in running this all the time? (no check for changed files)