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

[Bug] New --hide-prev-plan-comments flag fails on GitHub Enterprise #1009

Closed
chrisob opened this issue Apr 27, 2020 · 23 comments
Closed

[Bug] New --hide-prev-plan-comments flag fails on GitHub Enterprise #1009

chrisob opened this issue Apr 27, 2020 · 23 comments
Labels
bug Something isn't working

Comments

@chrisob
Copy link
Contributor

chrisob commented Apr 27, 2020

When enabling the new --hide-prev-plan-comments flag, Atlantis is unable to hide comments on GitHub Enterprise with the following error:

2020/04/27 12:31:09+0000 [EROR] command_runner.go:439 myorg/myrepo#91: Unable to hide old comments: minimize comment MDEyOklzc3VlQ29tbWVudDEyNDI3OQ==: non-200 OK status code: 406 Not Acceptable body: ""

Tested against GitHub Enterprise version 2.19, which, according to the documentation, supports the minimizeComment mutation as a preview. Looking at the code in PR #994, the Accept header seems to be set correctly as per the GH API preview requirement.

Unfortunately, --log-level debug doesn't print out the raw HTTP request/response, so I'm not really able to inspect the request/response bodies or headers.

Atlantis version: v0.12.0

@lkysow
Copy link
Member

lkysow commented Apr 27, 2020

@goodspark can you take a look?

@lkysow lkysow added the bug Something isn't working label Apr 27, 2020
@goodspark
Copy link

Not really. Because I don't have access to an account on Github Enterprise. That said, it looks like the minimize comment schema is now out of preview for github.com. But apparently still in preview for enterprise. So at the very least might want to add a temporary config to remove the preview schema accept header.

@chrisob
Copy link
Contributor Author

chrisob commented Apr 28, 2020

@goodspark @lkysow If I'm provided a docker image I'd be happy to test against my GHEE (also would be nice to have some extra debug info with raw HTTP to really see what's going on)

@goodspark
Copy link

Looking at the v4 graphQL clients, it doesn't look like a way to log the requests is exposed. What I can do is try removing the custom accept header on the off chance that GE is doing something weird and/or the docs are out of date.

goodspark pushed a commit to goodspark/atlantis that referenced this issue May 5, 2020
This is not part of the main API. This is to help test runatlantis#1009 For some reason API calls come back with '406 Not Acceptable Body' on Github Enterprise, despite the GE stating the custom Accept headers are required.

Maybe it's no longer required like on github.com and GH simply forgot to update the docs?
@goodspark
Copy link

I've pushed a change to https://github.com/goodspark/atlantis/tree/minimize-debug

Can you try making a Docker image from that fork+branch and trying it out?

It simply removes the custom Accept header for now.

@grimm26
Copy link
Contributor

grimm26 commented Jun 2, 2020

I'm running GHE 2.20.8 and I see a 422 in my atlantis log:

2020/06/02 14:53:53+0000 [EROR] command_runner.go:516 terraforms/production#3408: Unable to hide old comments: minimize comment MDEyOklzc3VlQ29tbWVudDcyODU3: non-200 OK status code: 422 Unprocessable Entity body:
....
Your browser did something unexpected. Please contact us if the problem persists.

@goodspark
Copy link

Was there any more information related to that error from your logs (before or after)?

The input schema docs for both the public and enterprise versions are fairly simple. The code is currently only passing the classifier (the reason for minimizing) and subjectId (the comment ID) as those are the only required fields (for both public and enterprise). There's one other field clientMutationId that's not required and is not being passed.

In addition, while the classifier enum values for public and enterprise are different, the value that's being used (OUTDATED) is valid for both.

So I'm not sure why we'd get back a response saying the body was unprocessable. Normally the raw response body is included in the error and log and could be helpful: https://github.com/Laisky/graphql/blob/laisky/graphql.go#L143-L146

@grimm26
Copy link
Contributor

grimm26 commented Jun 3, 2020

@goodspark the rest of the log message is just the error page from GHE. https://gist.github.com/grimm26/05159d6d10d629f7c12907a0bc324b0e

@grimm26
Copy link
Contributor

grimm26 commented Jun 3, 2020

is there a way to get atlantis to log the URL it is posting to to try to hide that comment?

@goodspark
Copy link

Sure, but I can tell you what it is right now: https://github.com/runatlantis/atlantis/blob/master/server/events/vcs/github_client.go#L54-L69

Looks like that might be wrong. Can you tell me what you're using for the Github Enterprise hostname in your Atlantis config?

I think based on the GE docs, the graphqlURL should really be fmt.Sprintf("https://%s/api/graphql", hostname). The confusion was the public Github API endpoint doesn't include api in the path and I wrote it against that initially. I can prep that fix but would like to see the config value you're using regardless.

goodspark pushed a commit to goodspark/atlantis that referenced this issue Jun 3, 2020
goodspark pushed a commit to goodspark/atlantis that referenced this issue Jun 3, 2020
goodspark pushed a commit to goodspark/atlantis that referenced this issue Jun 3, 2020
goodspark pushed a commit to goodspark/atlantis that referenced this issue Jun 3, 2020
@grimm26
Copy link
Contributor

grimm26 commented Jun 3, 2020

Sure, but I can tell you what it is right now: https://github.com/runatlantis/atlantis/blob/master/server/events/vcs/github_client.go#L54-L69

Looks like that might be wrong. Can you tell me what you're using for the Github Enterprise hostname in your Atlantis config?

I think based on the GE docs, the graphqlURL should really be fmt.Sprintf("https://%s/api/graphql", hostname). The confusion was the public Github API endpoint doesn't include api in the path and I wrote it against that initially. I can prep that fix but would like to see the config value you're using regardless.

Yep, that looks to be the issue. I have ATLANTIS_GH_HOSTNAME set to git.company.com so if the /api is not being appended with graphql. If I pull the /api out of a working graphql URL and curl it, I get that error page that atlantis is getting.

goodspark pushed a commit to goodspark/atlantis that referenced this issue Jun 3, 2020
Partially addresses runatlantis#1009, but doesn't fix it entirely. The 406 response
is still a puzzler.
@mveitas
Copy link

mveitas commented Jul 12, 2020

Running v0.14.0 I still get the following:

2020/07/12 00:44:01+0000 [EROR] command_runner.go:531 Development/my-live-repo#77: Unable to hide old comments: minimize comment MDEyOklzc3VlQ29tbWVudDc4MzA=: non-200 OK status code: 406 Not Acceptable body: ""

What is strange is that I build Atlantis locally (mac os) and connected it via ngrok to our GHE and saw that the hiding of previous plans was working as expected. This leads me to believe it could be something based on the build arch that is causing this?? I hope to have some time in the next few days to build this for the linux arch with some additional debugging to see if the theory holds.

@grimm26
Copy link
Contributor

grimm26 commented Jul 12, 2020

@mveitas see #1108 it is actually still broken in v0.14.0. If you were building from master locally and it worked, it is likely because of the fix that went in after v0.14.0

@mveitas
Copy link

mveitas commented Jul 13, 2020

@grimm26 thanks for the clarification. I missed that it got clobbered in another commit and fixed 3 days ago!

@lkysow
Copy link
Member

lkysow commented Jul 13, 2020 via email

@mveitas
Copy link

mveitas commented Aug 4, 2020

@lkysow Is there a possibility to cut a 0.14.1 release with the above fix so we can get this pushed to our production system?

@nitrocode
Copy link
Member

I'm using 0.14.0 and using Github saas with ATLANTIS_HIDE_PREV_PLAN_COMMENTS=true with ALANTIS_LOG_LEVEL=debug and when I use alantis plan -d some_directory multiple times, Atlantis does not hide the previous plan comments. Is this related or a separate issue?

@grimm26
Copy link
Contributor

grimm26 commented Aug 17, 2020

I'm using 0.14.0 and using Github saas with ATLANTIS_HIDE_PREV_PLAN_COMMENTS=true with ALANTIS_LOG_LEVEL=debug and when I use alantis plan -d some_directory multiple times, Atlantis does not hide the previous plan comments. Is this related or a separate issue?

That is this issue. The bug is fixed, but has not been released. I'm running latest instead of 0.14.0 in order to get the fix.

@nitrocode
Copy link
Member

Ah ok i thought this was exclusively for on prem installed github enterprise since a version was mentioned above whereas I'm using github.com enterprise saas.

@grimm26
Copy link
Contributor

grimm26 commented Aug 17, 2020

@nitrocode oh I missed that you said SaaS. I'm pretty sure it is the same URL pathing issue, though.

@lkysow
Copy link
Member

lkysow commented Aug 18, 2020

Closed by #1072

@lkysow lkysow closed this as completed Aug 18, 2020
@nitrocode
Copy link
Member

I'm unfortunately still seeing this with github.com unless I'm doing something wrong. I've created #1161 for 0.15.0 to track it.

@chrisob
Copy link
Contributor Author

chrisob commented Sep 7, 2020

I've finally updated to v0.15.0, big thanks to @goodspark for fixing this 😻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants