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

fix: correct the way that requested pro services are matched with enabled pro services #581

Open
wants to merge 106 commits into
base: feature/pro-sources
Choose a base branch
from

Conversation

linostar
Copy link

@linostar linostar commented Dec 3, 2024

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

What's done in this PR:

  • Update feature/pro-sources branch from main
  • Fix an issue that prevents building non-Pro rocks on Pro hosts in managed mode
  • Allow building Pro rocks as long as requested pro services are a subset of the enabled services in the build machine (the previous behavior was to only allow building Pro rocks if the requested services are exactly the same as the ones in the build machine)

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 91.50327% with 78 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature/pro-sources@238304b). Learn more about missing BASE report.

Files with missing lines Patch % Lines
craft_application/fetch.py 76.58% 46 Missing and 2 partials ⚠️
craft_application/services/fetch.py 87.05% 8 Missing and 3 partials ⚠️
craft_application/services/config.py 94.82% 3 Missing and 3 partials ⚠️
craft_application/commands/init.py 89.79% 5 Missing ⚠️
craft_application/services/init.py 96.77% 2 Missing and 1 partial ⚠️
craft_application/application.py 97.89% 1 Missing and 1 partial ⚠️
craft_application/commands/lifecycle.py 95.23% 0 Missing and 1 partial ⚠️
craft_application/launchpad/models/base.py 0.00% 0 Missing and 1 partial ⚠️
craft_application/util/pro_services.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##             feature/pro-sources     #581   +/-   ##
======================================================
  Coverage                       ?   90.49%           
======================================================
  Files                          ?       65           
  Lines                          ?     3787           
  Branches                       ?      414           
======================================================
  Hits                           ?     3427           
  Misses                         ?      289           
  Partials                       ?       71           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mattculler and others added 29 commits December 4, 2024 12:37
This is a fix for canonical/charmcraft#1874

The spec (ST105) allows <list-of-arch> | <arch> in both build-on and
build-for on all craft apps. This change makes the Platform model accept
a string value, but doesn't specify that in the schema. This means
text editors using the schema will still suggest converting the build-on
and build-for values to lists.
This fix addresses cases like, for example, a project that only builds on `amd64`
being built on `riscv64`. Previously the call to run_managed() would loop over
the (empty) build plan and then finish "successfully". This new version will
raise an EmptyBuildPlanError indicating to the user that they should check the
project's 'platforms' declaration and the command-line parameters.

Fixes canonical#225
`--shell` and `--shell-after` are implemented for a) consistency with the other
lifecycle commands, and b) because it makes sense to want to inspect the build
environment at packing time, considering that there are packing-related steps,
like writing the metadata file, that the users don't have a way to inspect
otherwise (short of examining the final artefact).

`--debug` is improved to actually shell into the build environment when the
packing itself failed, as opposed to the previous behavior of only debugging
failures that happen during the lifecycle steps.

Fixes canonical#430

---------

Co-authored-by: Alex Lowe <alex.lowe@canonical.com>
Adds a configuration service that can be used to get application
configuration.

CRAFT_* environment variables are only used if the config item
is known to craft-application (not for app-specific config).

Co-authored-by: Dariusz Duda <dariusz.duda@canonical.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…al#449)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* docs: add changelog for 4.2.0

* docs: review changelog 4.2.0

* Update docs/reference/changelog.rst

Co-authored-by: Michael DuBelko <michael.dubelko@gmail.com>

* move command entry to subheader

---------

Co-authored-by: Michael DuBelko <michael.dubelko@gmail.com>
This allows applications to override the inner run logic, with the end
goal of capturing application specific exceptions and raise
appropriate ones for Craft Application to handle.

Without the logic split, overriding run to capture exceptions is
virtually impossible as the run method holds a generic Exception
handler.

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
Previously, if a value was in the passed Namespace, it would return
that. The correct behaviour however is to check the environment for
the value and return that instead.
Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
Co-authored-by: Sergio Schvezov <sergio.schvezov@canonical.com>
renovate bot and others added 21 commits December 4, 2024 12:56
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
This lets commands be able to always rely on `self.config` existing, even
when running `fill_parser()` for help output.

Fixes canonical#530
Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Signed-off-by: Callahan Kovacs <callahankovacs@gmail.com>
Co-authored-by: Dariusz Duda <dariusz.duda@canonical.com>
Add some steps under the "Configuring fetch-service integration :: " progress,
to give some feedback that things are happening without all of the noisy output
from Apt.

Fixes canonical#536
Craft Application will only add default commands in a group if the application
doesn't define the command. The order of command groups is now preserved.

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Version 2.10.1 fixes the issue where the app greeting is shown twice when
running in managed mode, which happened because the Emitter was initialized in
"verbose" and then the Dispatcher set it to verbose again when parsing the
command line.

Fixes canonical#551
Signed-off-by: Dariusz Duda <dariusz.duda@canonical.com>
Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Both mypy and pyright complain that the "field.default_factory()" call has too
few arguments, even though its type is typing.Callable[[], Any]. It should be
fixed soon, and in the meantime we can ignore the spurious error.

Ref: pydantic/pydantic#10945
* Check that the provided "--name" is valid according to the standard name regex.

* If the user doesn't provide a "--name", *and* the project directory is *not*
  a valid project name, default to "my-project".
The issue with redirecting the fetch-service's output to a file via bash is
this: as a strict snap, the fetch-service cannot inherit the file descriptors
from the bash process if said process is spawned by a classic snap. This is
the case when the integration is controlled by a snapped craft tool:

```
+----------------------------+        +-------------+
|(snapped) craft-tool -> bash| -----> |fetch-service|
+----------------------------+        +-------------+
```

The solution here is to pass the path to the logfile to the fetch-service itself
through a new, recently implemented command-line option. We also update the
logpath to use tmp, and print the path when emitting the warning about the
integration being experimental.

Refs:

- https://bugs.launchpad.net/ubuntu/+source/snapd/+bug/1849753

Fixes canonical#550
@linostar linostar force-pushed the feature/pro-sources branch from 61f8604 to e6b1b47 Compare December 4, 2024 10:57
@linostar linostar force-pushed the feature/pro-sources branch 2 times, most recently from d46fef8 to 2130235 Compare December 4, 2024 13:20
@linostar linostar force-pushed the feature/pro-sources branch from 2130235 to 2d34504 Compare December 4, 2024 13:26
@linostar linostar changed the title Feature/pro sources fix: correct the way that requested pro services are matched with enabled pro services Dec 4, 2024
@@ -64,6 +64,8 @@ jobs:
- name: Release
uses: softprops/action-gh-release@v2
with:
# Generate release notes on the new GH release
generate_release_notes: true
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it comes from our changes to main, right? If so, could you separately merge main to this feature branch so the PR is cleaner?

Copy link
Author

Choose a reason for hiding this comment

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

@lengau I put the changes from main in seperate PR #582. Please merge that one first.

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.

7 participants