-
Notifications
You must be signed in to change notification settings - Fork 196
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
chore: refactor and simplify the config and script template #8902
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8902 +/- ##
==========================================
+ Coverage 60.19% 60.57% +0.37%
==========================================
Files 389 394 +5
Lines 47089 47754 +665
==========================================
+ Hits 28345 28925 +580
- Misses 16020 16075 +55
- Partials 2724 2754 +30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
408e76f
to
dae465e
Compare
dae465e
to
3178cd4
Compare
|
||
files := strings.Split(updated, ",") | ||
for _, item := range files { | ||
tokens := strings.Split(item, ":") |
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.
Characters related to parameters may be used with caution. Some special parameters can be any character, such as backup-related parameters:
archive_command = 'if [ $(date +%H%M) -eq 1200 ]; then rm -rf /home/postgres/pgdata/pgroot/arcwal/$(date -d"yesterday" +%Y%m%d); fi; mkdir -p /home/postgres/pgdata/pgroot/arcwal/$(date +%Y%m%d) && gzip -kqc %p > /home/postgres/pgdata/pgroot/arcwal/$(date +%Y%m%d)/%f.gz && sync /home/postgres/pgdata/pgroot/arcwal/$(date +%Y%m%d)/%f.gz'
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.
What is handled is the file name, not the file content.
a88f318
to
066ce2c
Compare
066ce2c
to
4e48aa5
Compare
AddAnnotationsInMap(synthesizedComp.StaticAnnotations). | ||
SetData(data). | ||
GetObject() | ||
return obj, nil |
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.
Need to set Component Ownership & Finalizer?
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.
Done.
487127a
to
d4c5993
Compare
// | ||
// - KB_CONFIG_FILES_CREATED: file1,file2... | ||
// - KB_CONFIG_FILES_REMOVED: file1,file2... | ||
// - KB_CONFIG_FILES_UPDATED: file1:checksum1,file2:checksum2... |
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.
should be file1@checksum1,...
or the implementation of templateFileChanges()
should be fixed.
// | ||
// - KB_CONFIG_FILES_CREATED: file1,file2... | ||
// - KB_CONFIG_FILES_REMOVED: file1,file2... | ||
// - KB_CONFIG_FILES_UPDATED: file1:checksum1,file2:checksum2... |
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.
ditto
for _, container := range synthesizedComp.PodSpec.Containers { | ||
for _, mount := range container.VolumeMounts { | ||
if mount.Name == volName { | ||
mountPath = mount.MountPath |
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.
what if there are multiple containers mounting the volume with different mount paths?
if len(tpl.Namespace) > 0 { | ||
return tpl.Namespace | ||
} | ||
return "default" |
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.
how about defaulting to the namespace of the cluster?
absPath := t.absoluteFilePath(transCtx, tplName, item) | ||
if len(absPath) > 0 { | ||
checksum := sha256.Sum256([]byte(pData[item])) | ||
items[2] = append(items[2], fmt.Sprintf("%s@%x", absPath, checksum)) |
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.
"@" => ":"
return s.handleExecAction(ctx, req, action) | ||
} | ||
|
||
func (s *actionService) precondition(ctx context.Context, req *proto.ActionRequest) error { | ||
return reconfigure(ctx, req) |
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.
the name is poor, maybe checkReconfigure
.
} | ||
|
||
func checkLocalFileUpToDate(file, checksum string) error { | ||
content, err := os.ReadFile(file) |
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.
I don't understand how it works, it seems that the volume is not mounted to the kbagent container, so kbagent could not see the files.
|
||
files := strings.Split(updated, ",") | ||
for _, item := range files { | ||
tokens := strings.Split(item, ":") |
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.
ditto
No description provided.