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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 17 commits into from
Aug 12, 2024
Merged

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

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 p-pichet force-pushed the master-gps-attributes branch from 8de13b9 to 8495deb Compare July 12, 2024 14:33
@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 p-pichet force-pushed the master-gps-attributes branch from 7175191 to d32e67e Compare August 2, 2024 09:49
@p-pichet p-pichet force-pushed the master-gps-attributes branch from d32e67e to 21a36c9 Compare August 5, 2024 06:31
@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 •
edited
Loading

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.

Steveb-p
Steveb-p reviewed Aug 10, 2024
View reviewed changes
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 p-pichet force-pushed the master-gps-attributes branch from 9ce3f7c to 10dea2a Compare August 12, 2024 09:08
@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 ?

Steveb-p
Steveb-p approved these changes Aug 12, 2024
View reviewed changes
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.

makasim
makasim reviewed Aug 12, 2024
View reviewed changes
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

docker-compose.yml Outdated
@@ -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 p-pichet requested review from makasim and Steveb-p August 12, 2024 12:13
@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

released https://github.com/php-enqueue/enqueue-dev/blob/master/CHANGELOG.md#01021-2024-08-12

@Steveb-p Steveb-p mentioned this pull request Nov 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makasim makasim Awaiting requested review from makasim

@Steveb-p Steveb-p Awaiting requested review from Steveb-p

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@p-pichet @dgafka @makasim @Steveb-p