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

Migrate Integrations to Storing Custom HTTP Headers by defining "fixtures_to_headers" methods. #12718

Closed
wants to merge 1 commit into from
Closed

Conversation

Hypro999
Copy link
Member

@Hypro999 Hypro999 commented Jul 5, 2019

This PR is basically an application of the work done in PR #12622 (In particular this PR addresses the request/suggestion made by @timabbott at the end of the discussion).

Here, for any integrations depending on custom http headers (which is usually just an event key) we add a fixtures_to_headers method which will determine the correct set of headers for a given fixture based on the fixture's name.

@Hypro999
Copy link
Member Author

Hypro999 commented Jul 9, 2019

@eeshangarg, could you please review this?

@timabbott
Copy link
Sponsor Member

I merged the simple ones. For gogs, we should just rename the files; that'd be cleaner.

For GitLab, what do you think about just using a modified version of the usual method?

We could rename e.g. push_multiple_committers.json to push_hook__multiple-commiters.json, and then have this:

def fixture_to_headers(fixture_name: str) -> Dict[str, Any]
    return fixture_name.split("__")[0].replace("_", " ").title()

@Hypro999
Copy link
Member Author

@timabbott, I made the change for Gogs (in retrospect, the GitHub way was the best approach for this one). I'm pretty hesitant about the suggestion for GitLab as a we'd mess up a lot of filenames in the process (making them really awkward).
E.g. confidential_issue_closed.json -> issue_hook__confidential_closed.json.
If you're sure that we should take such an approach then I'll do it.

@timabbott
Copy link
Sponsor Member

Hmm. I think issue_hook__confidential_issue_closed.json might be a better name for that one; we don't have to avoid redundancy if it makes the names more readable.

I'm not super worried about the filenames being long, especially in cases like this where it as adding clarity; text characters are cheap. :)

@timabbott
Copy link
Sponsor Member

Merged the gogs commit.

We use the same approach as was used for the GitHub integration.
@Hypro999
Copy link
Member Author

Hmm. I think issue_hook__confidential_issue_closed.json might be a better name for that one; we don't have to avoid redundancy if it makes the names more readable.
I'm not super worried about the filenames being long, especially in cases like this where it as adding clarity; text characters are cheap. :)

Cool! @timabbott, I've fixed the GitLab integration and now it follows a similar approach to the one used for the GitHub integration.

@timabbott
Copy link
Sponsor Member

Merged as fecf6a5, after adding a comment explaining the algorithm in the GitLab view.py function.

Huge thanks for fixing this @Hypro999!

Are there any remaining fixtures that don't work with the development panel, or can we declare complete victory for that project?

@timabbott timabbott closed this Jul 22, 2019
@Hypro999
Copy link
Member Author

Hypro999 commented Jul 22, 2019

@timabbott this PR migrated over every single integration that had custom headers and it did so without facing any issues. So I guess that we can declare as a "complete victory" for that project 😅. Worst case I made a typo somewhere and we'll need to fix that, but otherwise structurally everything worked out.

Thanks for reviewing and for your suggestions!

@Hypro999 Hypro999 deleted the migrate-integrations branch July 22, 2019 19:57
@timabbott
Copy link
Sponsor Member

timabbott commented Jul 22, 2019 via email

@Hypro999
Copy link
Member Author

Everything apart from the bitbucket3 change was perfect. There was a small changed needed for bitbucket3 which I've made in #12873.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants