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 failed state and test second deploy #8090

Closed
wants to merge 13 commits into from

Conversation

wouterdb
Copy link
Contributor

@wouterdb wouterdb commented Sep 13, 2024

Description

Basic end-to-end testing

close #8010

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

  • Attached issue to pull request
  • Changelog entry
  • Type annotations are present
  • Code is clear and sufficiently documented
  • No (preventable) type errors (check using make mypy or make mypy-diff)
  • Sufficient test cases (reproduces the bug/tests the requested feature)
  • Correct, in line with design
  • End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
  • If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)

@wouterdb wouterdb marked this pull request as ready for review September 13, 2024 12:03
@wouterdb wouterdb removed the request for review from Hugo-Inmanta September 13, 2024 12:09
Copy link
Contributor

@sanderr sanderr left a comment

Choose a reason for hiding this comment

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

Tiny review so far, pending discussion on Slack.


from inmanta.data.model import ResourceIdStr
from inmanta.resources import Id


@dataclass(frozen=True, kw_only=True)
class _Task(abc.ABC):
class Task(abc.ABC):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why still define the ABC here? I think it would make sense to move it to tasks as well, so that work depends on tasks but not vice versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, that could make things better

I found it more natural to have the abstract in one place, but I see the point

pass


class WithHashMatchTask(work.Task):
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this name come from? i.e. how does RefreshFact care about hash matching? From the implementation I see only the fact that they act on the latest version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may not be the best name, but given the plan to eliminate it, I would not invest too much time in it

Copy link
Contributor

Choose a reason for hiding this comment

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

the plan to eliminate it

Do you mean given my proposal on Slack? I'm not sure it would go in its entirety but we'll see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean that

@wouterdb wouterdb requested a review from sanderr September 13, 2024 13:57
src/inmanta/deploy/tasks.py Outdated Show resolved Hide resolved
"""Construct an agent that can execute using the resource container"""
agentmanager = server.get_slice(SLICE_AGENT_MANAGER)

# First part - test the ResourceScheduler (retrieval of data from DB)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this comment get lost? It doesn't seem to apply here.

@wouterdb wouterdb requested a review from sanderr September 13, 2024 16:08
@wouterdb wouterdb added the merge-tool-ready This ticket is ready to be merged in label Sep 16, 2024
@inmantaci
Copy link
Contributor

Processing this pull request

@inmantaci
Copy link
Contributor

Merged into branches master in 9ed614e

inmantaci pushed a commit that referenced this pull request Sep 16, 2024
# Description

Basic end-to-end testing

close #8010

# Self Check:

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [X] Attached issue to pull request
- [x] Changelog entry
- [x] Type annotations are present
- [x] Code is clear and sufficiently documented
- [x] No (preventable) type errors (check using make mypy or make mypy-diff)
- [x] Sufficient test cases (reproduces the bug/tests the requested feature)
- [x] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
- [ ] If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)
@inmantaci inmantaci closed this Sep 16, 2024
@inmantaci inmantaci deleted the issue/testing_all_features branch September 16, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-tool-ready This ticket is ready to be merged in
Projects
None yet
Development

Successfully merging this pull request may close these issues.

resource scheduler: agent instrumentation
3 participants