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

Linting, unit and integration tests pass for the frontend charm #16

Merged
merged 19 commits into from
Jun 6, 2023

Conversation

mz2
Copy link
Collaborator

@mz2 mz2 commented Jun 1, 2023

Addresses RTW-101: Adds a GitHub workflow which uses tox to run for the frontend charm...

  • linting & formatting checks (ruff & black)
  • unit tests
  • integration tests
  • changes to ubuntu:22.04 as base for the multi-stage Dockerfile for the frontend app (associated nginx config file path changes and charm changes)
  • fixes a bug in both backend and frontend charms, found by the unit testing effort: the frontend app needs to be restarted whenever the rest-api relation data changes, and rest-api relation data should change when backend hostname or port changes.

backend/charm/pyproject.toml Outdated Show resolved Hide resolved
frontend/charm/pyproject.toml Outdated Show resolved Hide resolved
@mz2 mz2 changed the title Linting, unit and integration tests pass for the frontend Linting, unit and integration tests pass for the frontend charm Jun 2, 2023
@mz2
Copy link
Collaborator Author

mz2 commented Jun 2, 2023

@nadzyah re: formatting, I agree with your comment but would prefer to follow up in a separate re-formatting PR when we have formatting checks with ruff and black across all the build targets in this repo and can put in place consistent settings (depending on what you use as a VS Code workspace would otherwise lead to inconsistent formatting).

Update: I did set things to 99 since actually that looks like the least disruptive setting. I have had a bunch of conflicting formatter plugins installed in VS Code on my workstations, which made my commits confused, apologies about that.

@mz2 mz2 force-pushed the frontend-charm-tests branch from dd50b57 to f02b021 Compare June 2, 2023 11:43
@mz2 mz2 marked this pull request as ready for review June 2, 2023 11:49

COPY --from=builder /app/build/web /usr/share/nginx/html
COPY nginx.conf /etc/nginx/conf.d/default.conf
COPY nginx.conf /etc/nginx/sites-available/test-observer-frontend
RUN ln -s /etc/nginx/sites-available/test-observer-frontend /etc/nginx/sites-enabled/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved to an Ubuntu base here beside the obvious reasons also because I noticed that pebble had problems with finding nginx from $PATH after I had moved to a multi-stage build (though it actually was present at /usr/sbin/nginx, so not sure what it was about actually), and this was not happening with Ubuntu as the base.

logger.debug(f"API hostname: {api_hostname} (app: {event.app})")

if self.unit.is_leader():
self._stored.backend_hostname = api_hostname
self._stored.backend_port = api_port
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Setting the port was missing.

@omar-selo
Copy link
Collaborator

Why not use the same linter for backend charm as the one used for backend code? If we think ruff is better than black we can go with it. But then I feel it's better to have one linter and one linter rule set for both of backend and backend charm

@omar-selo
Copy link
Collaborator

Why not use the same linter for backend charm as the one used for backend code? If we think ruff is better than black we can go with it. But then I feel it's better to have one linter and one linter rule set for both of backend and backend charm

Looks like ruff is compatible with black if the line lengths are the same (which is 88 for black by default)

@omar-selo
Copy link
Collaborator

omar-selo commented Jun 5, 2023

Why not use the same linter for backend charm as the one used for backend code? If we think ruff is better than black we can go with it. But then I feel it's better to have one linter and one linter rule set for both of backend and backend charm

Actually nvm, just noticed an issue as charm would use it's own virtual environment so linting wont work as some packages are specific to backend while others are to the charm

@mz2 mz2 merged commit e679dd6 into main Jun 6, 2023
@mz2 mz2 deleted the frontend-charm-tests branch June 6, 2023 09:39
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.

3 participants