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

✨(dimail) send domain creation request to dimail #454

Merged
merged 7 commits into from
Nov 14, 2024

Conversation

mjeammet
Copy link
Member

@mjeammet mjeammet commented Oct 9, 2024

Purpose

Send domain creation request to dimail when someone creates a domain in people.

Proposal

Description...

  • write function in dimail client
  • write an all-OK test
  • check actual dimail response upon sending only the "name" of the domain (and check which features must be enabled for valid domain)
    ...

@mjeammet mjeammet self-assigned this Oct 9, 2024
@mjeammet mjeammet force-pushed the mpj/dimail/send-request-domains-creation branch 2 times, most recently from 59a7c38 to e2c54de Compare October 17, 2024 14:12
@mjeammet mjeammet marked this pull request as ready for review October 17, 2024 14:16
@mjeammet mjeammet force-pushed the mpj/dimail/send-request-domains-creation branch from e2c54de to 874b085 Compare October 18, 2024 15:08
@mjeammet mjeammet force-pushed the mpj/dimail/send-request-domains-creation branch 2 times, most recently from de16b97 to a7a0168 Compare October 22, 2024 17:36

domain_name = "test.domain.fr"

with responses.RequestsMock() as rsps:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason, not to use the responses "classic" way? By classic I mean using the test function decorator responses.activate along with

    responses.add(
        responses.POST,
        url=re.compile(r".*/domains/"),
        json={
            "name": domain_name,
        },
        status=status.HTTP_201_CREATED,
    )

which seems (to me) a bit more readable.

Copy link
Member Author

@mjeammet mjeammet Oct 23, 2024

Choose a reason for hiding this comment

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

I haven't had the time to make this PR but I absolutely want to make a fixture to encapsulate these responses !

I don't know responses.activate but will try to investigate soon.

timeout=10,
)
except requests.exceptions.ConnectionError as error:
logger.error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

As error is an exception, why don't you use logger.exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely, I just didn't know there was a dedicated function but that's even better!

About this very line; I might change it for the error to be logged but the response to be passed as is (without exception).

@mjeammet mjeammet force-pushed the mpj/dimail/send-request-domains-creation branch from a7a0168 to 2187788 Compare October 23, 2024 16:49
@mjeammet mjeammet requested a review from qbey October 23, 2024 17:02
@mjeammet mjeammet force-pushed the mpj/dimail/send-request-domains-creation branch 4 times, most recently from 0afbe86 to ab0a4ef Compare October 30, 2024 14:52
@Morendil
Copy link
Collaborator

Morendil commented Nov 4, 2024

Seems that the only thing we need is to spin up the dimail container, so I propose to pick up this PR. @sdemagny if you want to pair on this ?

@Morendil
Copy link
Collaborator

Morendil commented Nov 5, 2024

Apparently it's not that simple ? I've tried adding the dimail container to the runtime dependencies in docker-compose, but we see the domain creation command failing with this error in the dimail logs: "You need to init the db by giving me a valid URL".

Further, it seems that the dimail image in the registry hasn't been updated in some time, so in CI we are testing against outdated code. @chrnin Any ideas ?

@Morendil Morendil force-pushed the mpj/dimail/send-request-domains-creation branch from ab0a4ef to 3617ab6 Compare November 5, 2024 10:45
@Morendil
Copy link
Collaborator

Morendil commented Nov 5, 2024

I think I have a pretty good idea now where this was going wrong. Not one single thing but a bunch of small fixes needed on both Régie and Dimail sides.

  • aim to have a "stable" dimail image (as opposed to "latest" which can be too cutting-edge) to code against
  • add dimail container to app-dev dependencies in Régie's docker-compose
  • use start-dev.sh for the starting command and not entrypoint.sh
  • on the Dimail side this start-dev.sh needs to listen on 0.0.0.0 not localhost
  • on Régie side, use the more recent schema for the POST /domains request body
  • fix the debug logging on Régie side, it's too scarce on details (and breaks owing to trying too hard with UTF-8)
  • on the Dimail side the DB was not being fully initialized in FAKE mode

So this needs coordination between an update to this PR and an MR on Dimail.

Stepping back from the nitty-gritty details, a bigger lesson for me is that unless coordination between Régie and Dimail development is more fluid (e.g. by trying cross-team pairing), the lead time for changes of this sort could be intolerably long: weeks to months…

@Morendil
Copy link
Collaborator

Morendil commented Nov 7, 2024

Fixes for Dimail submitted: https://gitlab.mim-libre.fr/dimail/dimail-api/-/merge_requests/51

@Morendil
Copy link
Collaborator

Morendil commented Nov 7, 2024

Fixes pushed to this PR for the other points. Waiting on the MR to be merged on dimail side.

@Morendil Morendil force-pushed the mpj/dimail/send-request-domains-creation branch 3 times, most recently from 743a6a5 to c4221ec Compare November 12, 2024 14:40
@Morendil Morendil force-pushed the mpj/dimail/send-request-domains-creation branch 4 times, most recently from a26833a to 076a702 Compare November 13, 2024 15:09
@Morendil
Copy link
Collaborator

Finally good to go 🎉 after fixing a few remaining tests and changing inter-container communication in CI to use Docker's internal networking between app and Dimail.

mjeammet and others added 7 commits November 14, 2024 18:08
Send domain creation request to dimail when someone creates a domain in people.
Remove duplicate and catch errors more gracefully. Fixes tests accordingly.
Start the Dimail container (in CI and local testing) when starting
the app. The pull_policy should have no effect on CI (because it starts
from a blank slate) but ensure we test against the most recent version
of the chosen tag.
Make testing easier in a local environment by adding Test credentials.
Adapt domain creationg request to latest protocol version, also
make error reporting more robust: don't assume utf-8 but use the
response's encoding, don't assume the error is JSON (it won't be
when getting a 500) but reproduce the whole thing instead.
To help debug with Dimail interop, save logs from the Dimail container.
Also fix the tests' expectations…
So that E2E tests in Github Actions can connect to Dimail container.
Previously we were attempting to connect as if from the outside. But
the E2E process is in fact inside the Docker Compose network.
("The tests came from inside the house !")
https://tvtropes.org/pmwiki/pmwiki.php/Main/TheCallsAreComingFromInsideTheHouse
@mjeammet mjeammet force-pushed the mpj/dimail/send-request-domains-creation branch from 076a702 to c294eb4 Compare November 14, 2024 17:09
@mjeammet mjeammet merged commit a10f65a into main Nov 14, 2024
19 of 20 checks passed
@mjeammet mjeammet deleted the mpj/dimail/send-request-domains-creation branch November 14, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants