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

Updates vendor dependencies #1762

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Updates vendor dependencies #1762

wants to merge 1 commit into from

Conversation

schnie
Copy link
Member

@schnie schnie commented Dec 12, 2024

Description

I'm currently working on cleaning up the output of our astro dev commands and plan to hide most of it behind our --verbosity flag by default. Our standard output will be much cleaner and will aim to not confuse our users, especially new ones who don't need to see a ton of docker layer SHAs getting echod out every time.

In order to sufficiently control output, we need a much newer version of the docker compose libraries. The currently used version directly attaches to os.Stdout and we can't override. New versions allow this to be specified and allow more control over the output.

This PR is the result of running go get -u and updating all our packages. One off updates were not working well for me. This gets our packages up to date and fixes the subsequent fallout from those upgrades in our code. It's the bare minimum changes needed to simply update the packages.

Follow up PRs will utilize this change and fine-tune the output to our needs.

I wish this was smaller, but I feel like we need to get up to date for many reasons. Changes mostly boil down to:

  • Scattered docker/compose type updates. New packages changed paths, names, etc of types. Mostly same functionally.
  • Updating the linter made it much more aggressive, so fixed a lot of the things it was failing on.
  • Updated mocks for new docker/compose types.

🎟 Issue(s)

Related to https://github.com/astronomer/astro/issues/26133

🧪 Functional Testing

This is a non-functional update, so I've only updated the tests to support the changes in types, etc. To test for regressions, I've run the following suites against this branch:

  • We've triggered the Astro e2e-stage-cli-version-matrix workflow on the Astro repo and pointed at this branch. The workflow can be viewed here. Note the switch to this branch in this step.

  • We've also run the new (currently in development) pytest astro dev suite against this branch with the following output:

❯ poetry run pytest -x -s -v dev_test.py
Configuration file exists at /Users/schnie/Library/Preferences/pypoetry, reusing this directory.

Consider moving TOML configuration files to /Users/schnie/Library/Application Support/pypoetry, as support for the legacy directory will be removed in an upcoming release.
=============================================================================================================================================== test session starts ===============================================================================================================================================
platform darwin -- Python 3.13.1, pytest-8.3.4, pluggy-1.5.0 -- /Users/schnie/repos/astro-cli/test/integration-test/venv/bin/python
cachedir: .pytest_cache
rootdir: /Users/schnie/repos/astro-cli/test/integration-test
configfile: pyproject.toml
collected 9 items                                                                                                                                                                                                                                                                                                 

dev_test.py::test_dev_init PASSED
dev_test.py::test_dev_start PASSED
dev_test.py::test_dev_ps PASSED
dev_test.py::test_dev_logs PASSED
dev_test.py::test_dev_parse PASSED
dev_test.py::test_dev_run PASSED
dev_test.py::test_dev_restart PASSED
dev_test.py::test_dev_export PASSED
dev_test.py::test_dev_kill PASSED

=============================================================================================================================================== 9 passed in 59.92s ================================================================================================================================================
  • Manual smoke testing locally

📸 Screenshots

Add screenshots to illustrate the validity of these changes.

📋 Checklist

  • Rebased from the main (or release if patching) branch (before testing)
  • Ran make test before taking out of draft
  • Ran make lint before taking out of draft
  • Added/updated applicable tests
  • Tested against Astro-API (if necessary).
  • Tested against Houston-API and Astronomer (if necessary).
  • Communicated to/tagged owners of respective clients potentially impacted by these changes.
  • Updated any related documentation

@schnie schnie force-pushed the update-compose-libs branch 6 times, most recently from 0347ee6 to 60051b8 Compare December 13, 2024 04:01
@schnie schnie force-pushed the update-compose-libs branch from 60051b8 to 11fc3ac Compare December 13, 2024 04:08
@schnie schnie marked this pull request as ready for review December 13, 2024 16:37
@@ -5,6 +5,7 @@ jobs:
working_directory: /go/src/github.com/astronomer/astro-cli
docker:
- image: quay.io/astronomer/ap-dind-golang:24.0.6
resource_class: large
Copy link
Member Author

Choose a reason for hiding this comment

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

Linting became a little heavier.

@@ -213,7 +213,7 @@ func (c *HTTPClient) DoAirflowClient(doOpts *httputil.DoOptions) (*Response, err
}
}

response, err := c.Do(doOpts)
response, err := c.Do(doOpts) //nolint:bodyclose
Copy link
Member Author

@schnie schnie Dec 13, 2024

Choose a reason for hiding this comment

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

@neel-astro @jeremybeard @pritt20 I ended up with a lot of these //nolint:bodyclose exceptions as I was making the updated linter setup happy. I really don't understand why its complaining though. In every single case we have a defer response.Body.Close() type statement right after the error check. I don't know how to make it happy haha. If anyone has an idea here, please let me know. Otherwise, I think we just ignore these (knowing that we are in fact closing the reader). We could keep the in-line versions, or we could just disable bodyclose in the linter completely. Curious what you think.

Comment on lines +1341 to +1355

// The latest versions of compose libs handle adding these labels at an outer layer,
// near the cobra entrypoint. Without these labels, compose will lose track of the containers its
// starting for a given project and downstream library calls will fail.
for name, s := range project.Services {
s.CustomLabels = map[string]string{
api.ProjectLabel: project.Name,
api.ServiceLabel: name,
api.VersionLabel: api.ComposeVersion,
api.WorkingDirLabel: project.WorkingDir,
api.ConfigFilesLabel: strings.Join(project.ComposeFiles, ","),
api.OneoffLabel: "False",
}
project.Services[name] = s
}
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

1 participant