0.5 - Feature scan endpoint directory #12

peteraba · 2018-04-10T09:12:45Z

PR #8 recreated.

…/apidemic into feature-scan-endpoint-directory

gernest

Please check the comment. I will take another look after the comments have been addressed.

gernest · 2018-04-10T09:52:03Z

README.md

@@ -158,6 +158,9 @@ There's also a 1% chance for the server to claim to be a [Teapot](https://tools.

 **Note**: JSON keys must be strings, providing your response codes as integers will not work!

+### Registering endpoints automatically
+The service will scan the `./endpoints/` directory relative to the currenct working directory on startup. You can change the directory to be scanned by adding a flag `--endpoint-dir=ENDPOINT_DIR`.


for relative paths endpoints/ will do, no need for ./endpoints/

it's true and I was considering it, but I feel ./endpoints is a bit more expressive. I'll change it but I'm not sure this will be an improvement.

gernest · 2018-04-10T09:55:04Z

helper.go

+	"net/http"
+)
+
+func JsonRequest(method string, path string, body interface{}) *http.Request {


Do you plan to use this anywhere else apart from tests?

This only works in tests so no need to export the name or put it in not *_test.go file. And if in case you want to use this in something else you better add godoc comment since it is an exported API to say what it does.

This is already used when reading the endpoints. https://github.com/gernest/apidemic/pull/12/files#diff-510e51effef9a268bfb7ca60921904daR88

Okay, just add comments then

gernest · 2018-04-10T09:56:27Z

cmd/apidemic/main.go

+				cli.StringFlag{
+					Name:   "endpoint-dir",
+					Usage:  "Directory to scan for endpoint",
+					Value:  "./endpoints/",


endpoints/

gernest · 2018-04-10T10:03:12Z

cmd/apidemic/main.go

 	s := apidemic.NewServer()

+	err := addEndpoints(s, endpointDir)


The endpoint dir should be optional. Not everyone wants to load stuff from a dir. I think the workflow should be like this.

If the user provided a path to the endoints then use it to lad endpoint and exit if any error occured.

If the use didn't provide an endpoint , try to check if endpoints/ is there if not continue execution ignore the loading . In case the dir is there ☝️ see above bullet but use endpoints/ instead.

Else just ignore the loading

gernest · 2018-04-10T10:04:07Z

cmd/apidemic/main.go

+		req := apidemic.JsonRequest("POST", "/register", registerPayload)
+		s.ServeHTTP(w, req)
+
+		log.Printf("%s is registered\n", filePath)


No need, for this. It just adds noise.

Actually I found this very useful and only adds noice at the beginning. Maybe I can add a verbose flag to see this log?

I see, you can leave this for now.

gernest · 2018-04-10T10:04:33Z

cmd/apidemic/main.go

+		log.Printf("%s is registered\n", filePath)
+
+		if w.Code != http.StatusOK {
+			return errors.New("registering " + filePath + " failed")


use fmt.Errorf

peteraba · 2018-04-10T11:19:03Z

@gernest please check my responses before I fix the remaining issues.

peteraba · 2018-04-10T20:26:37Z

@gernest please check my changes.

peteraba · 2018-04-12T11:18:25Z

@gernest any chance you could check this?

gernest · 2018-04-12T12:51:01Z

Hey sorry. I am checking it right now.

gernest

Okay looks nice. I left one comment. I noticed there are no new tests for the addEndpoints function.

gernest · 2018-04-12T12:52:47Z

cmd/apidemic/main.go

 	s := apidemic.NewServer()

+	_, err := os.Stat(endpointDir)
+	if err != nil && endpointDir != "endpoints/" {
+		fmt.Errorf("Endpoint not found: %s", endpointDir)


no caps in error message. Should be endpoint not found

gernest · 2018-04-12T12:59:40Z

cmd/apidemic/main.go

+			continue
+		}
+
+		filePath := endpointDir + file.Name()


use filepath.Join(endpointDir ,file.Name()) for cross platform use

peteraba · 2018-04-12T22:22:37Z

@gernest I fixed the issues you pointed out in the code change.

I did not add tests for the addEndpoints function as there's currently none for the main package. If you really want to see those added, please give me some pointers on what exactly should be tested.

peteraba · 2018-04-15T14:44:31Z

@gernest would be cool to have these PRs closed as soon as possible. I won't be opening more anytime soon I think 😉

peteraba · 2018-04-17T09:11:04Z

@gernest ?

gernest · 2018-04-17T09:18:48Z

@peteraba I'm sorry. I am very sick right now. I will take another look the moment I recover and feel better.

peteraba · 2018-04-17T09:47:51Z

@gernest understood, get well soon!

string(3127) "MongoDB\Driver\Exception\BulkWriteException: Updating the path 'message.reply_markup.inline_keyboard.0' would create a conflict at 'message.reply_markup.inline_keyboard' in /app/vendor/mongodb/mongodb/src/Operation/Update.php:179 app_1 | Stack trace: app_1 | #0 /app/vendor/mongodb/mongodb/src/Operation/Update.php(179): MongoDB\Driver\Server->executeBulkWrite('avita.pvm_token', Object(MongoDB\Driver\BulkWrite), Array) app_1 | #1 /app/vendor/mongodb/mongodb/src/Operation/UpdateOne.php(105): MongoDB\Operation\Update->execute(Object(MongoDB\Driver\Server)) app_1 | #2 /app/vendor/mongodb/mongodb/src/Collection.php(1033): MongoDB\Operation\UpdateOne->execute(Object(MongoDB\Driver\Server)) app_1 | #3 /app/vendor/formapro/yadm/src/Storage.php(161): MongoDB\Collection->updateOne(Array, Array, Array) app_1 | #4 /app/vendor/formapro/pvm/src/Yadm/StandaloneTokenDAL.php(98): Formapro\Yadm\Storage->update(Object(App\Model\PvmToken)) app_1 | #5 /app/vendor/formapro/pvm/src/ProcessEngine.php(289): Formapro\Pvm\Yadm\StandaloneTokenDAL->persistToken(Object(App\Model\PvmToken)) app_1 | #6 /app/vendor/formapro/pvm/src/ProcessEngine.php(217): Formapro\Pvm\ProcessEngine->persistToken(Object(App\Model\PvmToken)) app_1 | gernest#7 /app/vendor/formapro/pvm/src/ProcessEngine.php(251): Formapro\Pvm\ProcessEngine->doProceed(Object(App\Model\PvmToken)) app_1 | gernest#8 /app/vendor/formapro/pvm/src/ProcessEngine.php(188): Formapro\Pvm\ProcessEngine->transition(Object(App\Model\PvmToken)) app_1 | gernest#9 /app/vendor/formapro/pvm/src/ProcessEngine.php(78): Formapro\Pvm\ProcessEngine->doProceed(Object(App\Model\PvmToken)) app_1 | gernest#10 /app/src/Service/PvmHandler.php(132): Formapro\Pvm\ProcessEngine->proceed(Object(App\Model\PvmToken)) app_1 | gernest#11 [internal function]: App\Service\PvmHandler->App\Service\{closure}(Object(App\Model\PvmToken), Object(App\Storage\PvmTokenStorage)) app_1 | gernest#12 /app/vendor/formapro/yadm/src/Storage.php(300): call_user_func(Object(Closure), Object(App\Model\PvmToken), Object(App\Storage\PvmTokenStorage)) app_1 | gernest#13 /app/src/Storage/PvmStorage.php(70): Formapro\Yadm\Storage->lock(Object(MongoDB\BSON\ObjectId), Object(Closure)) app_1 | #14 /app/src/Service/PvmHandler.php(137): App\Storage\PvmStorage->lockToken(Object(App\Model\PvmToken), Object(Closure)) app_1 | #15 /app/src/Service/PvmHandler.php(114): App\Service\PvmHandler->handleToken(Object(App\Model\PvmToken)) app_1 | #16 /app/src/Controller/TelegramUpdatesHookController.php(31): App\Service\PvmHandler->handleUpdate(Object(Formapro\TelegramBot\Update)) app_1 | #17 /app/vendor/symfony/http-kernel/HttpKernel.php(150): App\Controller\TelegramUpdatesHookController->__invoke(Object(Symfony\Component\HttpFoundation\Request), '725382688:AAFZQ...', Object(Formapro\TelegramBot\Bot), Object(App\Service\PvmHandler), Object(Symfony\Bridge\Monolog\Logger)) app_1 | #18 /app/vendor/symfony/http-kernel/HttpKernel.php(67): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1) app_1 | #19 /app/vendor/symfony/http-kernel/Kernel.php(198): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) app_1 | #20 /app/public/index.php(25): Symfony\Component\HttpKernel\Kernel->handle(Object(Symfony\Component\HttpFoundation\Request)) app_1 | #21 {main}"

peteraba and others added 5 commits April 10, 2018 00:09

Scan endpoint directory on startup

f9ef8e9

Scan endpoint directory on startup

3ee84be

Merge branch 'feature-scan-endpoint-directory' of github.com:peteraba…

f87684b

…/apidemic into feature-scan-endpoint-directory

Fix merge issue in tests

eed1a04

Self-review fix

6c54b88

peteraba changed the title ~~Feature scan endpoint directory~~ 0.5 - Feature scan endpoint directory Apr 10, 2018

peteraba mentioned this pull request Apr 10, 2018

0.7 - Feature support list of entities response #10

Open

gernest suggested changes Apr 10, 2018

View reviewed changes

Review fixes

4578eb6

gernest suggested changes Apr 12, 2018

View reviewed changes

Review fixes 2

cc0970a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

0.5 - Feature scan endpoint directory #12

0.5 - Feature scan endpoint directory #12

peteraba commented Apr 10, 2018

gernest left a comment

gernest Apr 10, 2018

peteraba Apr 10, 2018

gernest Apr 10, 2018

peteraba Apr 10, 2018

gernest Apr 10, 2018

gernest Apr 10, 2018

peteraba Apr 10, 2018

gernest Apr 10, 2018

peteraba Apr 10, 2018

gernest Apr 10, 2018

peteraba Apr 10, 2018

gernest Apr 10, 2018

gernest Apr 10, 2018

peteraba commented Apr 10, 2018

peteraba commented Apr 10, 2018

peteraba commented Apr 12, 2018

gernest commented Apr 12, 2018

gernest left a comment

gernest Apr 12, 2018

gernest Apr 12, 2018

peteraba commented Apr 12, 2018 •

edited

Loading

peteraba commented Apr 15, 2018

peteraba commented Apr 17, 2018

gernest commented Apr 17, 2018

peteraba commented Apr 17, 2018

		s := apidemic.NewServer()

		err := addEndpoints(s, endpointDir)

0.5 - Feature scan endpoint directory #12

Are you sure you want to change the base?

0.5 - Feature scan endpoint directory #12

Conversation

peteraba commented Apr 10, 2018

gernest 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteraba commented Apr 10, 2018

peteraba commented Apr 10, 2018

peteraba commented Apr 12, 2018

gernest commented Apr 12, 2018

gernest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteraba commented Apr 12, 2018 • edited Loading

peteraba commented Apr 15, 2018

peteraba commented Apr 17, 2018

gernest commented Apr 17, 2018

peteraba commented Apr 17, 2018

peteraba commented Apr 12, 2018 •

edited

Loading