Skip to content

Support Amazon Kinesis Data Streams Service #1008

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
Merged
merged 3 commits into from
Jul 13, 2021
Merged

Support Amazon Kinesis Data Streams Service #1008

merged 3 commits into from
Jul 13, 2021

Conversation

GrahamCampbell
Copy link
Contributor

NB The generator command was unable to generate DescribeStream:

In StructureShape.php line 29:
                                                          
  [InvalidArgumentException]                              
  The member "StreamDescription.Shards" does not exists.  
                                                          

Exception trace:
  at /data/src/CodeGenerator/src/Definition/StructureShape.php:29
 AsyncAws\CodeGenerator\Definition\StructureShape->getMember() at /data/src/CodeGenerator/src/Generator/PaginationGenerator.php:105
 AsyncAws\CodeGenerator\Generator\PaginationGenerator->generateOutputPagination() at /data/src/CodeGenerator/src/Generator/PaginationGenerator.php:74
 AsyncAws\CodeGenerator\Generator\PaginationGenerator->generate() at /data/src/CodeGenerator/src/Generator/OperationGenerator.php:122
 AsyncAws\CodeGenerator\Generator\OperationGenerator->generate() at /data/src/CodeGenerator/src/Command/GenerateCommand.php:269
 AsyncAws\CodeGenerator\Command\GenerateCommand->generateService() at /data/src/CodeGenerator/src/Command/GenerateCommand.php:147
 AsyncAws\CodeGenerator\Command\GenerateCommand->generateServicesSequential() at /data/src/CodeGenerator/src/Command/GenerateCommand.php:84
 AsyncAws\CodeGenerator\Command\GenerateCommand->execute() at /data/vendor/symfony/console/Command/Command.php:256
 Symfony\Component\Console\Command\Command->run() at /data/vendor/symfony/console/Application.php:971
 Symfony\Component\Console\Application->doRunCommand() at /data/vendor/symfony/console/Application.php:290
 Symfony\Component\Console\Application->doRun() at /data/vendor/symfony/console/Application.php:166
 Symfony\Component\Console\Application->run() at /data/src/CodeGenerator/generate.php:20
 require() at /data/generate:32

@GrahamCampbell GrahamCampbell marked this pull request as draft May 16, 2021 11:03
@jderusse
Copy link
Member

NB The generator command was unable to generate DescribeStream:

Indeed, this is the first service that have pagination over nested objects.
I'll take care of this tomorrow.

jderusse
jderusse reviewed May 17, 2021
View reviewed changes
@jderusse
Copy link
Member

You should now be able to rebase on top of master and add the operation DescribeStream

@GrahamCampbell
Copy link
Contributor Author

I can do at the weekend. :)

@jderusse
Copy link
Member

I've rebased and push -force your PR to fix tests (and a bug in pagination).

@jderusse jderusse force-pushed the kinesis branch 5 times, most recently from 1296680 to 06527b7 Compare May 29, 2021 22:19
@jderusse jderusse marked this pull request as ready for review May 30, 2021 09:56
@GrahamCampbell
Copy link
Contributor Author

Nice, thanks!

Nyholm
Nyholm reviewed May 31, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Could we make the changed in the code generator in a different PR?

manifest.json
"StopStreamEncryption",
"StreamExists",
"StreamNotExists",
"UpdateShardCount"
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I'm not yet sure what I need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt I need any of the methods like IncreaseStreamRetentionPeriod since that would be managed by cloudformation.

@theophileds
Copy link

Hello,

I was about to contribute to Kinesis support until I found out that someone already did!
Are there any blockers on this pull request?

@Nyholm
Copy link
Member

Nyholm commented Jun 22, 2021

There are two things:

  1. Separate the code generator changes into a new PR
  2. Only generate the operations needed. I doubt we need all operations. But if you do, then that is okey =)

@GrahamCampbell
Copy link
Contributor Author

I am still not really sure what options I need, but I'd be happy to go with a very minimal selection, then add in some additional ones if required. @jderusse knows the state of this PR the best at the moment. I'm not sure if he had to make any manual adjustments to the generated code?

Nyholm added a commit to Nyholm/aws that referenced this pull request Jul 2, 2021
@Nyholm
CodeGenerator changes from async-aws#1008
ba1627e
@Nyholm Nyholm mentioned this pull request Jul 2, 2021
Merged
Nyholm added a commit that referenced this pull request Jul 2, 2021
@Nyholm
CodeGenerator changes from #1008 (#1040)
1f5fd24 
* CodeGenerator changes from #1008

* Regenerate all
@Nyholm
Copy link
Member

Nyholm commented Jul 2, 2021

Could you please rebase this PR?
I've separated the CodeGenerator stuff in #1040.

@jderusse jderusse force-pushed the kinesis branch 3 times, most recently from 3c7384f to 961d945 Compare July 5, 2021 21:29
GrahamCampbell
GrahamCampbell commented Jul 5, 2021
View reviewed changes
src/Service/Kinesis/src/Enum/EncryptionType.php
@@ -3,10 +3,10 @@
namespace AsyncAws\Kinesis\Enum;

/**
* The encryption type used. This value is one of the following:.
* The server-side encryption type used on the stream. This parameter can be one of the following values:.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

php-cs-fixer incorrectly changed this doc. it should actually be

The server-side encryption type used on the stream.

This parameter can be one of the following values:

 - `NONE`: Do not encrypt the records in the stream.
 - `KMS`: Use server-side encryption on the records in the stream using a customer-managed AWS KMS key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note in particular the lack of a period after the colon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

php-cs-fixer incorrectly changed this doc.

well, more that we were miss-using the short description area of the phpdoc. it is only meant to span either one line or one sentence.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, but it's not related to this PR.
The code is generated, then cs-fixed, We can fix this later

@jderusse
Copy link
Member

jderusse commented Jul 5, 2021

Phpstan and psalm errors are not related to this PR and are addressed in #1041

This PR si RFR for my side.

@Nyholm Nyholm merged commit accf316 into async-aws:master Jul 13, 2021
@Nyholm
Copy link
Member

Nyholm commented Jul 13, 2021

Thank you.

@GrahamCampbell GrahamCampbell deleted the kinesis branch July 13, 2021 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jderusse jderusse jderusse left review comments

@Nyholm Nyholm Nyholm approved these changes

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
@GrahamCampbell @jderusse @theophileds @Nyholm