Skip to content

Implement token and orgs validation - #66

Closed
smacker wants to merge 2 commits into
src-d:masterfrom
smacker:orgs_validation
Closed

Implement token and orgs validation #66
smacker wants to merge 2 commits into
src-d:masterfrom
smacker:orgs_validation

Conversation

@smacker

@smacker smacker commented Jun 20, 2019

Copy link
Copy Markdown
Contributor

Fix: #59
Based on: #63
Only the last commit is new.

This PR fixes the issue according to the agreement but I have doubts it's the best way to validate stuff.

Current problems:

  • workdir get created before validation
  • even with --no-deps docker-compose already creates network and volume
  • in case of wrong organization there will be workdir with this wrong org
  • we can't prune automatically on error because init can be actually "re-init" and prune would delete everything

I would better re-work it using simple http requests to github in the cli itself before starting messing with docker-compose. Or use docker instead of docker-compose. WDYT?

smacker added 2 commits June 20, 2019 12:36
Fix: #61

Signed-off-by: Maxim Sukharev <max@smacker.ru>
Fix: #59

Signed-off-by: Maxim Sukharev <max@smacker.ru>
@smacker
smacker requested a review from a team June 20, 2019 13:30
@se7entyse7en

Copy link
Copy Markdown
Contributor

workdir get created before validation

I guess that there's something preventing it, but why can't you just check it immediately?

Or use docker instead of docker-compose.

I'd go with docker instead of docker-compose to avoid having extra things such as those that you mentioned (volumes and networks) and also to avoid calling github and ending with similar logic here and in ghsync.

@smacker

smacker commented Jun 20, 2019

Copy link
Copy Markdown
Contributor Author

I guess that there's something preventing it, but why can't you just check it immediately?

you can't use docker-compose commands without creating workdir.

@se7entyse7en

Copy link
Copy Markdown
Contributor

you can't use docker-compose commands without creating workdir.

Oh, right. So using docker should also fix this problem?

@smacker

smacker commented Jun 20, 2019

Copy link
Copy Markdown
Contributor Author

yes. I provided 2 solutions for the problems in description:

  • http requests from the cli
  • docker

@carlosms

Copy link
Copy Markdown
Contributor

But if we run docker, where do we get the name & version of the image? Is it possible to get it from docker-compose?
It would be inconvenient to have ghsync:v1.0 in the docker-compose.yml, and have our CLI use the ghsync:latest image. It may download 2 different images.

@carlosms

Copy link
Copy Markdown
Contributor

I'm also OK moving the code from ghsync validate to here, if it makes things cleaner.

@dpordomingo

dpordomingo commented Jun 20, 2019

Copy link
Copy Markdown
Contributor

I also agree that it would be better if we could copy-paste the code in charge of the validation from ghsync to this cli command.

I'd not delete that functionality from ghsync, because it also provides value there (with a small change)

@smacker

smacker commented Jun 20, 2019

Copy link
Copy Markdown
Contributor Author

Using docker would require parsing of docker-compose.yml file.
But I would prefer to go with validation in the cli for now. It's simpler and more robust.
We can find a better solution after the release.

@se7entyse7en please confirm you are ok with it and I'll redo this PR.

@se7entyse7en

Copy link
Copy Markdown
Contributor

@smacker It's ok for me, I don't have a strong preference 👍

@smacker smacker closed this Jun 20, 2019
@se7entyse7en

Copy link
Copy Markdown
Contributor

We can find a better solution after the release.

And yes, we're kind of in-rush now 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add github token and orgs validation

4 participants