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

Add version info to each generated file #144

Merged
merged 2 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions commands/jinja2_tags.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we call this file something which better describes it's reason for being?

Maybe generated_by_tags.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's fine as it is. If it grows and requires refactoring, then so be it. It's similar to the Django convention of putting views in a views.py file, models in a models.py file etc.

Copy link
Contributor

@WillGibson WillGibson Aug 23, 2023

Choose a reason for hiding this comment

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

Actually, version_tag.py, to match the class name, would be best.

Copy link
Contributor

Choose a reason for hiding this comment

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

jinja2 tags are a standard. It's self explanatory and follows conventions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everywhere I have worked for over a decade it has been the convention to name a file containing a class to match the class name.

Are you able to link to the standard you mention?

Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds like you've spent too much time working with Java; my experience has been the opposite for the past 10+ years working mainly in python land.

It's worth reading the modules section of this python structure guide: https://docs.python-guide.org/writing/structure/#modules

Maybe we should start a team style guide and come to a consensus on these smaller issues, so that we can increase the objectivity of code reviews?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignoring the side languages, the last ten years went PHP > TypeScript > Java & Ruby.

Now Python.

I am 100% up for a team style guide. I'll start a thread in Slack to get the ball rolling.

Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import datetime
from importlib.metadata import PackageNotFoundError
from importlib.metadata import version

from jinja2_simple_tags import StandaloneTag


class VersionTag(StandaloneTag):
tags = {"version_info"}

def _get_version(self):
try:
return version("dbt-copilot-tools")
except PackageNotFoundError:
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to test this? Looks like we are testing the happy path via fixtures already, but we also handle this exception and return "UNKNOWN".

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be an edge case - it only happens if the code is run directly, instead of after installing the package. I did consider manually parsing pyproject.toml to grab the version, but I don't see much value in it. Your --version command also has the same issue.

Copy link
Member

Choose a reason for hiding this comment

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

The reason why I mention it is because we have a similar method here that does parse pyproject.toml: https://github.com/uktrade/copilot-tools/blob/main/utils/check_pypi.py#L43

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh cool, I'll raise another PR and fix it in both places.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I went down a rabbit hole here. I wasn't liking the way Poetry installs the package as a dependency so at one point I manually removed it, which then caused the PackageNotFound exception.

I did some digging yesterday and found this: python-poetry/poetry#800, which explains why poetry works this way.

But the upshot of it is, there's no scenario that version() should fail and require a fall back to parsing the toml file. So I'm just going to remove the try/except block.

Copy link
Member

Choose a reason for hiding this comment

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

Yeahhh, I was thinking we would ideally never run into that exception anyway. It might be worth bumping up the version as well, so this change gets published on to PyPI as well.

return "UNKNOWN"

def render(self):
format = "%Y-%m-%d %H:%M:%S"
time = datetime.datetime.now().strftime(format)
return f"Generated by copilot-helper {self._get_version()} / {time}"
1 change: 1 addition & 0 deletions commands/templates/addons/env/addons.parameters.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
# At present, this may include parameters which are not used in your application's addons.
# If you get a "parse environment addons: template does not require the parameter "{parameter_name}" in parameters file" error,
# just delete the offending parameter from this file.
Expand Down
1 change: 1 addition & 0 deletions commands/templates/addons/env/aurora-postgres.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
Parameters:
# Copilot required Parameters...
App:
Expand Down
1 change: 1 addition & 0 deletions commands/templates/addons/env/opensearch.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
Parameters:
# Copilot required Parameters...
App:
Expand Down
1 change: 1 addition & 0 deletions commands/templates/addons/env/rds-postgres.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
Parameters:
# Copilot required Parameters...
App:
Expand Down
1 change: 1 addition & 0 deletions commands/templates/addons/env/redis-cluster.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
Parameters:
# Copilot required Parameters...
App:
Expand Down
1 change: 1 addition & 0 deletions commands/templates/addons/env/s3.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
Parameters:
# Copilot required Parameters...
App:
Expand Down
1 change: 1 addition & 0 deletions commands/templates/addons/svc/appconfig-ipfilter.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
Parameters:
App:
Type: String
Expand Down
1 change: 1 addition & 0 deletions commands/templates/addons/svc/s3-policy.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
Metadata:
cfn-lint:
config:
Expand Down
1 change: 1 addition & 0 deletions commands/templates/env/manifest.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
# The manifest for the "{{ name }}" environment.
# Read the full specification for the "Environment" type at:
# https://aws.github.io/copilot-cli/docs/manifest/environment/
Expand Down
1 change: 1 addition & 0 deletions commands/templates/svc/manifest-backend.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
# The manifest for the "{{ name }}" service.
# Read the full specification for the "Backend Service" type at:
# https://aws.github.io/copilot-cli/docs/manifest/backend-service/
Expand Down
1 change: 1 addition & 0 deletions commands/templates/svc/manifest-public.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# {% version_info %}
# The manifest for the "{{ name }}" service.
# Read the full specification for the "Load Balanced Web Service" type at:
# https://aws.github.io/copilot-cli/docs/manifest/lb-web-service/
Expand Down
2 changes: 2 additions & 0 deletions commands/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import jinja2

from commands.exceptions import ValidationException
from commands.jinja2_tags import VersionTag

SSM_BASE_PATH = "/copilot/{app}/{env}/secrets/"
SSM_PATH = "/copilot/{app}/{env}/secrets/{name}"
Expand Down Expand Up @@ -128,6 +129,7 @@ def setup_templates():
Path(__file__).parent.parent / Path("templates")
templateLoader = jinja2.PackageLoader("commands")
templateEnv = jinja2.Environment(loader=templateLoader, keep_trailing_newline=True)
templateEnv.add_extension(VersionTag)

return templateEnv

Expand Down
18 changes: 16 additions & 2 deletions poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ cfn-flip = "1.3.0"
aiohttp = "^3.8.4"
certifi = "^2023.07.22"
cryptography = "^41.0.3"
jinja2-simple-tags = "^0.5.0"

[tool.poetry.group.dev.dependencies]
cfn-lint = "^0.77.7"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
# The manifest for the "development" environment.
# Read the full specification for the "Environment" type at:
# https://aws.github.io/copilot-cli/docs/manifest/environment/
Expand Down
1 change: 1 addition & 0 deletions tests/fixtures/make_addons/config/copilot/web/manifest.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
# The manifest for the "web" service.
# Read the full specification for the "Load Balanced Web Service" type at:
# https://aws.github.io/copilot-cli/docs/manifest/lb-web-service/
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
# At present, this may include parameters which are not used in your application's addons.
# If you get a "parse environment addons: template does not require the parameter "{parameter_name}" in parameters file" error,
# just delete the offending parameter from this file.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
Parameters:
# Copilot required Parameters...
App:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
Parameters:
# Copilot required Parameters...
App:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
Parameters:
# Copilot required Parameters...
App:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
Parameters:
# Copilot required Parameters...
App:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
Parameters:
# Copilot required Parameters...
App:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
Parameters:
App:
Type: String
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
Metadata:
cfn-lint:
config:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
Metadata:
cfn-lint:
config:
Expand Down
1 change: 1 addition & 0 deletions tests/fixtures/production_environment_manifest.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
# The manifest for the "production" environment.
# Read the full specification for the "Environment" type at:
# https://aws.github.io/copilot-cli/docs/manifest/environment/
Expand Down
1 change: 1 addition & 0 deletions tests/fixtures/test_environment_manifest.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
# The manifest for the "test" environment.
# Read the full specification for the "Environment" type at:
# https://aws.github.io/copilot-cli/docs/manifest/environment/
Expand Down
1 change: 1 addition & 0 deletions tests/fixtures/test_service_manifest.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Generated by copilot-helper v0.1-TEST / 2023-08-22 16:00:00
# The manifest for the "test-service" service.
# Read the full specification for the "Load Balanced Web Service" type at:
# https://aws.github.io/copilot-cli/docs/manifest/lb-web-service/
Expand Down
4 changes: 4 additions & 0 deletions tests/test_bootstrap_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import shutil
from pathlib import Path
from unittest.mock import MagicMock
from unittest.mock import Mock
from unittest.mock import call
from unittest.mock import patch

Expand All @@ -10,6 +11,7 @@
import yaml
from click.testing import CliRunner
from cloudfoundry_client.common_objects import JsonObject
from freezegun import freeze_time
from moto import mock_ssm
from moto import mock_sts
from schema import SchemaError
Expand Down Expand Up @@ -88,6 +90,8 @@ def test_load_and_validate_config_invalid_file():
)


@freeze_time("2023-08-22 16:00:00")
@patch("commands.jinja2_tags.version", new=Mock(return_value="v0.1-TEST"))
def test_make_config(tmp_path):
"""Test that make_config generates the expected directories and file
contents."""
Expand Down
5 changes: 5 additions & 0 deletions tests/test_copilot_cli.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import os
import shutil
from pathlib import Path
from unittest.mock import Mock
from unittest.mock import patch

import boto3
import pytest
from click.testing import CliRunner
from freezegun import freeze_time
from moto import mock_ssm

from commands.copilot_cli import copilot as cli
Expand Down Expand Up @@ -83,6 +86,8 @@ class TestMakeAddonCommand:
("aurora_addons.yml", ["my-aurora-db.yml", "addons.parameters.yml"], ["appconfig-ipfilter.yml"], True),
],
)
@freeze_time("2023-08-22 16:00:00")
@patch("commands.jinja2_tags.version", new=Mock(return_value="v0.1-TEST"))
def test_make_addons_success(
self, tmp_path, addon_file, expected_env_addons, expected_service_addons, expect_db_warning
):
Expand Down