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

adds support to have nowUTCToken #866

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

Conversation

tomParty
Copy link

@tomParty tomParty commented Dec 2, 2023

4d47b55 was needed as there were refernces to atomic.Bool which is introduced in 1.19 and currently mod was set to 1.18

4d47b55 introduces nowUTCToken to set all instances in a payload to the same now in utc

Copy link
Member

@ldemailly ldemailly left a comment

Choose a reason for hiding this comment

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

The string nowUTC is a bit odd - I am wondering also if the format is a bit arbitrary, what if I want to insert unix timestamps or something else

so maybe... {custom} calling a custom function passed in instead? (and that function can return time.Now().UTC().String() for your use case but something else for someone else?

nowUTC := nowFn().UTC()
body := string(c.body)
for strings.Contains(body, nowUTCToken) {
body = strings.Replace(body, nowUTCToken, nowUTC.String(), 1)
Copy link
Member

Choose a reason for hiding this comment

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

.String() once, outside of the loop

but why not replaceall

Copy link
Author

Choose a reason for hiding this comment

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

yea sounds good i'l just use replaceall, I think I was trying to just keep similar footprint as was was done for uuid

Copy link
Member

Choose a reason for hiding this comment

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

maybe uuid can use fixing too :) that also was contributed and I might have missed it in the then review

Copy link
Author

Choose a reason for hiding this comment

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

hahah i'll look into it. I think I remember seeing uuid are unique.. so if you had a payload with multiple, they'd be all different..

@@ -513,12 +517,25 @@ func (c *Client) StreamFetch(ctx context.Context) (int, int64, uint) {
for strings.Contains(body, uuidToken) {
body = strings.Replace(body, uuidToken, generateUUID(), 1)
}
c.body = []byte(body)
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed but wasn't for {uuid}

Copy link
Author

Choose a reason for hiding this comment

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

when there's a body that has both payloads it would overwrite the req.Body without the {uuid} interpolation. so I made sure that we updated it before processing down the chain

@tomParty
Copy link
Author

tomParty commented Dec 2, 2023

The string nowUTC is a bit odd - I am wondering also if the format is a bit arbitrary, what if I want to insert unix timestamps or something else

so maybe... {custom} calling a custom function passed in instead? (and that function can return time.Now().UTC().String() for your use case but something else for someone else?

This would be awesome. I'm curious how we'd register the custom func or exec. or you imagining something like

{time.Now().UTC().String()}
{custom | time.Now().UTC().String()}

@ldemailly
Copy link
Member

The string nowUTC is a bit odd - I am wondering also if the format is a bit arbitrary, what if I want to insert unix timestamps or something else
so maybe... {custom} calling a custom function passed in instead? (and that function can return time.Now().UTC().String() for your use case but something else for someone else?

This would be awesome. I'm curious how we'd register the custom func or exec. or you imagining something like

{time.Now().UTC().String()}
{custom | time.Now().UTC().String()}

Well I was thinking in the library case one can set what "custom" function is in the HTTPClientOption, but for the default cli that's indeed a good question how it could be used; we could have a flag with a few built in (unixts,utc,sequence,...) as I don't think go can support compiling at run time - on the other hand... that's what templates are also?

@ldemailly
Copy link
Member

We could have a couple of variants and use templates with a function map also but it wouldn't be fully dynamic (kinda by design of template safety)

@tomParty
Copy link
Author

tomParty commented Dec 7, 2023

We could have a couple of variants and use templates with a function map also but it wouldn't be fully dynamic (kinda by design of template safety)

yea, hoping to have some time tomorrow or this weekend to tinker around with it more, at a min, I might just add a set of functions/tokens like you mentioned before unixts,utc,sequence,..

@ldemailly
Copy link
Member

Btw one way to do extensibility could be https://github.com/ldemailly/go-scratch/blob/main/plugin_example/extensible_main.go

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

Successfully merging this pull request may close these issues.

None yet

2 participants