Decaf endpoints #223

alextwoods · 2024-12-16T23:45:06Z

Description of changes:

Changes in smithy-client:

Remove Endpoints plugin - this is instead code generated per service.
Add rules engine stdlib functions. (copied from hearth).
Add jmespath as a dependency (used by endpoint parameter bindings).

Changes in smithy:

Codegeneration for:
- endpoint parameters
- endpoint provider/resolver
- endpoint plugin
- endpoint provider specs (translate endpoint test cases).
extension points for register endpoint built in bindings and stdlib functions.
default weld that adds default endpoint rules/tests.
All Endpoint2.0 test models.
Task to generate sdk per test model + run specs (added to CI).

Not part of this PR (future work):

OperationInput tests (ie, E2E endpoint tests of the client. Requires being able to make client operation calls)
Endpoints + Auth. (requires us to implement Auth schemes/resolver, ect).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

gems/smithy/lib/smithy/anvil/endpoint_parameter.rb

gems/smithy/lib/smithy/anvil/templates/client/plugins/endpoint.erb

gems/smithy/lib/smithy/anvil/views/client/endpoint_parameters.rb

gems/smithy/lib/smithy/anvil/views/client/module.rb

mullermp · 2024-12-17T00:12:25Z

gems/smithy/lib/smithy/vise/endpoints/built_in_binding.rb

+
+      # Endpoint BuiltInBinding
+      class BuiltInBinding
+        def initialize(id:, render_config:, render_build:, render_test_set:)


I've just been using options = {} everywhere because of how easy it is to just pass options down the line without extracting keyword args... Not sure how you feel about that. I think I lean towards keyword args for required public interfaces.

Yeah - I think for required public interfaces I'd generally prefer keyword args. Since this is an extension point/public interface, I think keeping them as keywords make sense.

I don't think this is public at all. Please just use options for vise usage. But I think these are better used as view classes now - since vise does not have any shape wrapping.

These must be public - these are the extension point for users to add endpoint related bindings.

I've removed the classes and just using arrays/hashes.

gems/smithy/lib/smithy/welds/endpoints.rb

mullermp · 2024-12-17T00:14:28Z

gems/smithy/lib/smithy/welds/endpoints.rb

+        [Vise::Endpoints::BuiltInBinding.new(
+          id: 'SDK::Endpoint',
+          render_config: proc do |_plan|
+            <<-ADD_OPTION


I've been using ~ with indentation. I think you can make this a method that takes option values. Custom options come from the endpoint params but static values for endpoint.

I can switch to <<~ for indentation - I don't have a style preference on those.

What do you mean about having this method take option values. I'm not quite sure I understand.

In general, this needs to be fairly flexible to support all of our existing/future built ins (eg: account id).

This still needs to be indented. What I was saying before was to have a method that is responsible for rending the config, but this is fine to hardcode endpoint for now. Eventually this will need to be a method.

This is the correct indenting for it to render correctly in the generated code.

Why should this be a method? I think I'm still a little confused about that. Using a proc/callable is what lets a user define arbitrary configuration(s) that may apply for the builtin. I think thats perferable to coming up with a codegen time modeling of config.

gems/smithy/lib/smithy/welds/endpoints.rb

jterapin

May want to run rubocop to fix some indentations.

jterapin · 2024-12-17T16:31:16Z

gems/smithy-client/lib/smithy-client/endpoint_rules.rb

+
+        attr_reader :scheme, :authority, :path, :normalized_path, :is_ip
+
+        def as_json(_options = {})


Curious to know why _options = {} is necessary than omitting it. I'm guessing it is due to the way this class is used as a "wrapper" for URI?

good question, I guess we could use * or ... instead to indicate it can take arguments, but doesn't use them.

Edit, I've updated to use just *.

gems/smithy-client/lib/smithy-client/endpoint_rules.rb

gems/smithy/lib/smithy/anvil/endpoint_parameter.rb

gems/smithy/lib/smithy/anvil/templates/client/plugins/endpoint.erb

gems/smithy/lib/smithy/anvil/views/client/endpoint_parameters.rb

gems/smithy/lib/smithy/anvil/views/client/plugins/endpoint.rb

gems/smithy/lib/smithy/tools/underscore.rb

.github/workflows/ci.yml

Rakefile

gems/smithy-client/lib/smithy-client/endpoint_rules.rb

gems/smithy-client/spec/smithy-client/endpoint_rules_spec.rb

mullermp · 2024-12-27T14:39:03Z

gems/smithy/lib/smithy/welds/endpoints.rb

+        [Vise::Endpoints::BuiltInBinding.new(
+          id: 'SDK::Endpoint',
+          render_config: proc do |_plan|
+            <<-ADD_OPTION


This still needs to be indented. What I was saying before was to have a method that is responsible for rending the config, but this is fine to hardcode endpoint for now. Eventually this will need to be a method.

gems/smithy/lib/smithy/welds/endpoints.rb

mullermp · 2024-12-27T14:42:06Z

gems/smithy/spec/fixtures/endpoints/endpoints-string-array/model.json

@@ -0,0 +1,319 @@
+{


Are there .smithy file equivalents we should copy in too?

I guess - they're in AwsDrSeps. I'm not sure I like this mixed model.json/model.smithy we have going for our test fixtures, though I'm not quite sure what we should do.

I find it easier to edit the model files in smithy IDL then generate the json with it, that's why both are there. We could require smithy cli and shell out to convert to AST and not have the json, but also having the json inspectable (in the folder) helps with writing generation.

gems/smithy/spec/interfaces/client/endpoint_provider_spec.rb

# Conflicts: # Rakefile

mullermp

Nice!

mullermp · 2024-12-30T20:13:37Z

Rakefile

+    include_paths = []
+    tmp_dirs = []
+    Dir.glob('gems/smithy/spec/fixtures/endpoints/*/model.json') do |model_path|
+      test_name = model_path.split('/')[-2]


Is there a better way to express this one?

Do you mean the model_path.split('/')[-2] part? I could do model_path.split('/').last(2).first instead, but that felt worse. Alternatively we coudl use regex to extract it, but I think splitting on / is simpler here.

Was thinking in terms of readability. What exactly is extracted? It's otherwise fine.

gems/smithy-client/lib/smithy-client.rb

gems/smithy-client/lib/smithy-client/base.rb

gems/smithy-client/spec/smithy-client/endpoint_rules_spec.rb

gems/smithy/lib/smithy/anvil/client/endpoint_parameter.rb

mullermp · 2024-12-30T20:25:45Z

gems/smithy/lib/smithy/forge/client.rb

@@ -24,6 +24,7 @@ def forge

      private

+      # rubocop:disable Metrics/AbcSize


I added the methods here to solve this but I guess it was just deferred. Is there a way to solve this one? Maybe if enumerator is passed to a method and that method does the yielding? Otherwise I guess it's fine to just ignore this one as it's a giant list.

Yeah, its just a list so I don't think rubucop is useful for that... We could solve it by breaking it into even more methods, but I dont think that really helps readability.

Yeah. In that case the multimethods are kinda unnecessary.

mullermp · 2024-12-30T20:26:16Z

gems/smithy/lib/smithy/forge/client.rb

+          e.yield "lib/#{@gem_name}/plugins/endpoint.rb", render_endpoint_plugin
+
+          e.yield 'spec/spec_helper.rb', render_spec_helper
+          e.yield 'spec/endpoint_provider_spec.rb', render_endpoint_provider_spec


I think by convention this should still be spec/gemname/specs

Was following what we had in V3, but I can update.

mullermp · 2024-12-30T20:27:39Z

gems/smithy/lib/smithy/welds/endpoints.rb

+      def endpoint_built_in_bindings
+        {
+          'SDK::Endpoint' => {
+            # Text indenting is used in generated view.


It would be better if indenting was relative instead of absolute here. So, using <<~ADD_OPTION with one level of indenting, then in our view, apply the current indenting level plus this one level.

I'm struggling with this because of the multiline text. Is there a way to have erb views indent multiline text?

gems/smithy/spec/fixtures/endpoints/endpoint-bindings.smithy

smithy-build.json

alextwoods added 5 commits December 13, 2024 13:14

Port hearth endpoint rules module

bcfc937

Add bindings to codegen

355a83a

Generation of endpoint parameters

43e4caa

Skeleton endpoint provider

9ef8233

endpoint plugin with basic config

784ff6a

alextwoods requested a review from mullermp December 16, 2024 23:45

mullermp reviewed Dec 17, 2024

View reviewed changes

Merge branch 'decaf' into decaf_endpoints

fe46555

jterapin reviewed Dec 17, 2024

View reviewed changes

alextwoods added 21 commits December 17, 2024 10:42

PR Cleanups

9724b49

More pr cleanups

8d791d0

Merge branch 'decaf' into decaf_endpoints

599d4aa

Rubocop fixes

f00e788

Fix specs

6de786b

Merge branch 'decaf' into decaf_endpoints

6f5b086

Rubocop

b9e8bbc

Add generated resolver spec (templates/view, not working yet)

79b7b14

Some rubocop cleanups

ba682bb

Merge branch 'decaf' into decaf_endpoints

dae2110

Endpoint spec generation

b3f5fe4

Merge branch 'decaf' into decaf_endpoints

5a4be49

endpoint provider interface spec

4ebf372

WIP - add handler code + per operation parameters

9a52cb1

Merge branch 'decaf' into decaf_endpoints

3feea81

Possible weld spec fix

cb91d68

Merge branch 'decaf' into decaf_endpoints

97bfc55

Add new endpoint test model + fix issues in trait usage

a40a16f

Generate create for operation parameters

6bc541f

Merge branch 'decaf' into decaf_endpoints

313dc33

Working parameters spec for operations

959762b

alextwoods added 3 commits December 26, 2024 09:22

rubocop

0a23ea5

Run all endpoint specs

2e39864

fix rubocop

96c1cc7

alextwoods changed the title ~~[WIP]Decaf endpoints~~ Decaf endpoints Dec 26, 2024

alextwoods marked this pull request as ready for review December 26, 2024 18:51

mullermp reviewed Dec 27, 2024

View reviewed changes

mullermp and others added 8 commits December 27, 2024 13:32

Simplify spec task

418d80f

PR cleanups

d94f1c1

# Conflicts: # Rakefile

Run all endpoint specs together

af7daa6

Merge branch 'decaf' into decaf_endpoints

fd882a9

Remove endpoint bindings from plan. Remove endpoint binding classes

9117b45

Add smithy files for endpoint fixtures

1c38695

Merge branch 'decaf' into decaf_endpoints

bf448d1

fix merge issues

a98c0e0

mullermp approved these changes Dec 30, 2024

View reviewed changes

alextwoods added 3 commits December 30, 2024 13:07

PR feedback

956d432

generated rubocop fixes

f1a9a60

Rubocop cleanups

0d31124

alextwoods merged commit 74e8854 into decaf Dec 31, 2024
3 checks passed

alextwoods deleted the decaf_endpoints branch December 31, 2024 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decaf endpoints #223

Decaf endpoints #223

alextwoods commented Dec 16, 2024 •

edited

Loading

mullermp Dec 17, 2024

alextwoods Dec 17, 2024

mullermp Dec 27, 2024

alextwoods Dec 27, 2024

alextwoods Dec 30, 2024

mullermp Dec 17, 2024

alextwoods Dec 17, 2024

mullermp Dec 27, 2024

alextwoods Dec 27, 2024

jterapin left a comment

jterapin Dec 17, 2024

alextwoods Dec 17, 2024 •

edited

Loading

mullermp Dec 27, 2024

mullermp Dec 27, 2024

alextwoods Dec 27, 2024

mullermp Dec 27, 2024

mullermp left a comment

mullermp Dec 30, 2024

alextwoods Dec 30, 2024

mullermp Dec 30, 2024

mullermp Dec 30, 2024

alextwoods Dec 30, 2024

mullermp Dec 30, 2024

mullermp Dec 30, 2024

alextwoods Dec 30, 2024

mullermp Dec 30, 2024

alextwoods Dec 30, 2024


		attr_reader :scheme, :authority, :path, :normalized_path, :is_ip

		def as_json(_options = {})

		@@ -24,6 +24,7 @@ def forge

		private

		# rubocop:disable Metrics/AbcSize

Decaf endpoints #223

Decaf endpoints #223

Conversation

alextwoods commented Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jterapin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextwoods Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mullermp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextwoods commented Dec 16, 2024 •

edited

Loading

alextwoods Dec 17, 2024 •

edited

Loading