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

feat(GPS): allow send attributes in Google PubSub message. #1349

Merged
merged 17 commits into from
Aug 12, 2024

Conversation

p-pichet
Copy link
Contributor

@p-pichet p-pichet commented May 6, 2024

Allow to send PubSub attributes

@p-pichet
Copy link
Contributor Author

@makasim Can someone look at this PR please ?

@p-pichet
Copy link
Contributor Author

Hello @dgafka.
Can you review this PR or tell me who can ?

@dgafka
Copy link
Contributor

dgafka commented Jun 20, 2024

Hello @p-pichet
I am not maintainer of this package, @makasim is ;)

@p-pichet
Copy link
Contributor Author

as i saw you approve some requests and i don't have any responses, i ask you but i understand.

@p-pichet
Copy link
Contributor Author

p-pichet commented Jul 2, 2024

@makasim

@makasim
Copy link
Member

makasim commented Jul 2, 2024

ci pipeline does not trigger for this PR. I dont know why. I cannot merge PR without it being tested on CI succesfully.

@p-pichet
Copy link
Contributor Author

p-pichet commented Jul 2, 2024

ci pipeline does not trigger for this PR. I dont know why. I cannot merge PR without it being tested on CI succesfully.

i see why the CI is not trigger.
You are agree with the modification ?

@p-pichet
Copy link
Contributor Author

p-pichet commented Jul 2, 2024

I don't understand how to run CI.

@p-pichet
Copy link
Contributor Author

p-pichet commented Jul 2, 2024

Hi @makasim, can you run CI actions, please?

@makasim
Copy link
Member

makasim commented Jul 2, 2024

I cannot, I would do it if it were possible

@p-pichet
Copy link
Contributor Author

p-pichet commented Jul 2, 2024

I hesitate if it was a trigger message.
Do you know the condition to enable the CI ?
Is it because my repository is not correctly setup ?

@makasim
Copy link
Member

makasim commented Jul 2, 2024

CI has to be triggered on any MR by default without any additional actions from any side. There is one exception where a maintainer has to approve CI but there is none.

@makasim
Copy link
Member

makasim commented Jul 2, 2024

Maybe try open a new PR from new branch, hope it helps

@p-pichet
Copy link
Contributor Author

p-pichet commented Jul 2, 2024

@makasim i push a fix, can you run the CI please.

@p-pichet
Copy link
Contributor Author

p-pichet commented Jul 5, 2024

@makasim i push a change, can you run the CI please.

1 similar comment
@p-pichet
Copy link
Contributor Author

@makasim i push a change, can you run the CI please.

@p-pichet
Copy link
Contributor Author

p-pichet commented Aug 1, 2024

@makasim I push a change, can you run the CI please.
For the version 8.x of PHP, we got issue on the SQS, but I don't touch it. Who can have a look at it ?

@p-pichet
Copy link
Contributor Author

p-pichet commented Aug 1, 2024

@makasim We found that the 404 error come from an update of aws/aws-sdk-php.
Who is in charge of the maintenance of external lib ?

@p-pichet
Copy link
Contributor Author

p-pichet commented Aug 6, 2024

@makasim I push a change, can you run the CI please.

@p-pichet
Copy link
Contributor Author

p-pichet commented Aug 6, 2024

@makasim it's seem the image use to run CI not found the docker compose command.

@p-pichet
Copy link
Contributor Author

p-pichet commented Aug 6, 2024

@makasim I push a change, can you run the CI please.

@p-pichet
Copy link
Contributor Author

p-pichet commented Aug 8, 2024

@Steveb-p i try something can you run the CI ?

@p-pichet
Copy link
Contributor Author

p-pichet commented Aug 9, 2024

@Steveb-p and @makasim, the CI fail on the SQS and SNS.
We don't have strong knowledge to understand what happen.
do you have a name to someone can help use ?
Maybe @ASKozienko as you work on the failing test.

@p-pichet
Copy link
Contributor Author

p-pichet commented Aug 9, 2024

@Steveb-p can you run the CI please ?

@p-pichet
Copy link
Contributor Author

p-pichet commented Aug 9, 2024

Thanks @Steveb-p
The CI is green.
Who can merge it ?

@Steveb-p
Copy link
Contributor

Steveb-p commented Aug 9, 2024

Thanks @Steveb-p The CI is green. Who can merge it ?

Let me review the code after work, just in case.

pkg/gps/Tests/GpsProducerTest.php Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
docs/transport/gps.md Outdated Show resolved Hide resolved
pkg/gps/GpsMessage.php Show resolved Hide resolved
@p-pichet
Copy link
Contributor Author

@Steveb-p can you run the CI and take a new look on the code please ?

@p-pichet p-pichet requested a review from Steveb-p August 12, 2024 08:47
@p-pichet
Copy link
Contributor Author

@Steveb-p, I push the missing code style.
Can you run the CI please ?
And as you validate the PR, what I need to do for its merge ?

Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

LGTM.

I will leave it for a while before merging, probably for a day.

Unless there are any comments from @makasim or anyone else.

pkg/sns/composer.json Outdated Show resolved Hide resolved
pkg/sqs/composer.json Outdated Show resolved Hide resolved
@p-pichet
Copy link
Contributor Author

Forgot the part to get the attributes on receive a message.

bin/changelog Outdated
#git add CHANGELOG.md && git commit -m "Release $1" -S && git push origin "$CURRENT_BRANCH"
Copy link
Member

Choose a reason for hiding this comment

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

uncomment please, I need it while releasing new tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry

@@ -127,13 +127,13 @@ services:
- '9090:9090'

localstack:
image: 'localstack/localstack:0.8.10'
image: 'localstack/localstack'
Copy link
Member

Choose a reason for hiding this comment

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

set a fixed version please

@makasim
Copy link
Member

makasim commented Aug 12, 2024

Few comments from me, Almost there.

@p-pichet
Copy link
Contributor Author

@Steveb-p and @makasim i push a fix for your comment and a code to get attributes for received.
Can you run the CI and validate the PR please ?

@makasim makasim merged commit 6b48a41 into php-enqueue:master Aug 12, 2024
34 checks passed
@makasim
Copy link
Member

makasim commented Aug 12, 2024

@p-pichet big thank you for your tireless work. Enqueue's CI is green and it not only provides the feature you need but unblocks a lot of other stuck MRs.

Kudos to you, much appreciated!

@p-pichet
Copy link
Contributor Author

@p-pichet big thank you for your tireless work. Enqueue's CI is green and it not only provides the feature you need but unblocks a lot of other stuck MRs.

Kudos to you, much appreciated!

Thanks a lot.
When do you think the release will be created ?

@makasim
Copy link
Member

makasim commented Aug 13, 2024

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.

4 participants