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

Store both web & api configuration variables in the same file (api.yml) #360

Merged
merged 17 commits into from Jun 27, 2019

Conversation

kamaev
Copy link
Contributor

@kamaev kamaev commented May 8, 2019

No description provided.

@kamaev kamaev requested a review from borovskyav May 8, 2019 13:39
@coveralls
Copy link

coveralls commented May 8, 2019

Coverage Status

Coverage remained the same at 79.74% when pulling d9de4e8 on feature/web_config_external into dc7d2ef on master.

@kamaev kamaev requested a review from Pliner May 27, 2019 11:51
@beevee beevee added this to Review in 2.5.1 via automation Jun 10, 2019
Copy link
Contributor

@Pliner Pliner left a comment

Choose a reason for hiding this comment

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

Кажется, что после этого PR перестанет работать локальный docker-compose и надо бы это поправить.

@kamaev
Copy link
Contributor Author

kamaev commented Jun 18, 2019

Кажется или перестанет?

@Pliner
Copy link
Contributor

Pliner commented Jun 18, 2019

Кажется или перестанет?

Сейчас в нем используется web_config.json, который ты удалил. Вероятно, нужно настройки и в локальном docker-compose поменять, чтобы все работало так же, как до твоей ветки, то есть со включенными remote триггерами.

@Pliner
Copy link
Contributor

Pliner commented Jun 18, 2019

Кажется или перестанет?

Сейчас в нем используется web_config.json, который ты удалил. Вероятно, нужно настройки и в локальном docker-compose поменять, чтобы все работало так же, как до твоей ветки, то есть со включенными remote триггерами.

Хотя, я проверил и вроде все даже будет работать. В любом случае нужно удалить этот конфиг из папки local, а так же перестать его подключать в docker-compose.

Copy link
Contributor

@Pliner Pliner left a comment

Choose a reason for hiding this comment

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

Спасибо за фикс после rebase. У меня не осталось никаких вопросов, кроме того, который я @borovskyav задал.

@@ -36,7 +35,9 @@ func NewHandler(db moira.Database, log moira.Logger, index moira.Searcher, confi

router.Route("/api", func(router chi.Router) {
router.Use(moiramiddle.DatabaseContext(database))
router.Get("/config", webConfig(configFile))
router.Get("/web", web(webConfig))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Pliner @borovskyav нет по вот этому вопросов?
я пока впилил обратно совместимые изменения, чтобы уже потом изменить путь
мне кажется занимать путь 'config' только для валидации контактов будет жирновато
хотя если мы будем сюда же потом втаскивать управление сендерами из api/web - то наверное тогда лучше оставить как было?

@kamaev kamaev merged commit 2df9e43 into master Jun 27, 2019
2.5.1 automation moved this from Review to Done Jun 27, 2019
@kamaev kamaev deleted the feature/web_config_external branch June 27, 2019 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2.5.1
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants