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

Remove visible todos from lsm doc #7201

Closed
wants to merge 14 commits into from
Closed

Conversation

FloLey
Copy link
Contributor

@FloLey FloLey commented Feb 15, 2024

Description

add docs/ remove todos
follow-up ticket:

  1. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/inmanta-core/7202
  2. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/std/504
  3. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/apt/281

closes #7134

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)

@FloLey FloLey changed the title update Remove visible todos from lsm doc. Feb 15, 2024
@FloLey FloLey changed the title Remove visible todos from lsm doc. Remove visible todos from lsm doc Feb 15, 2024
Copy link
Contributor

@arnaudsjs arnaudsjs left a comment

Choose a reason for hiding this comment

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

@@ -10,8 +10,6 @@ In the network, this virtual wire is implemented as a VXlan tunnel, tied to both
Each such tunnel requires a "VXLAN Network Identifier (VNI)" that uniquely identifies the tunnel.
In the allocation phase, the orchestrator selects a VNI and ensures no other customer is assigned the same VNI.

.. todo: add picture of service
Copy link
Contributor

Choose a reason for hiding this comment

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

This TODO is not mentioned in the follow-up ticket that was created.

docs/lsm/index.rst Outdated Show resolved Hide resolved
TODO: Three set of attributes
Each service instance defines 3 sets of its attributes that are used in different situations:

- ``candidate_attributes``: The set of attributes used when the service instance model state is candidate or designed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should explain this in term of the model state. The model state is a purely internal state of the orchestrator. You cannot see this state on the web-console for example. I think we should explain this more from the end-user perspective. This attribute set is mainly used to validate a change before activating (deploying) it.

Each service instance defines 3 sets of its attributes that are used in different situations:

- ``candidate_attributes``: The set of attributes used when the service instance model state is candidate or designed.
- ``active_attributes``: The set of attributes when the model is evaluated with only active instances.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not True. These attributes are also used when a validation compile is done for another service instance for example.

Comment on lines 130 to 132
- ``rollback_attributes``: The set of attributes that can be used to rollback active attributes that result in an error
state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ``rollback_attributes``: The set of attributes that can be used to rollback active attributes that result in an error
state.
- ``rollback_attributes``: The set of attributes that can be used to rollback active attributes that result in an error state.

Comment on lines 67 to 85
entity InterfaceIPAssignment extends lsm::ServiceEntity:
"""
Interface details.

Updating service entities
=========================
:attr router_ip: The IP address of the SR linux router that should be configured.
:attr router_name: The name of the SR linux router that should be configured.
:attr interface_name: The name of the interface of the router that should be configured.
:attr address: The IP-address to assign to the given interface.
"""

std::ipv_any_address router_ip
string router_name
string interface_name

std::ipv_any_interface address
lsm::attribute_modifier address__modifier="rw+"
end

implement InterfaceIPAssignment using parents
Copy link
Contributor

Choose a reason for hiding this comment

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

When we include a model in the docs, we usually add the model to a separate file. Then we can use that file to add a test case that verifies whether the model actually compiles and we can use the same file to include the model (or part of it) in the docs. We should do the same here. Otherwise the code included in the docs might no longer compile due to future changes to the lsm module or compiler.

Co-authored-by: arnaudsjs <2684622+arnaudsjs@users.noreply.github.com>
@FloLey FloLey added the merge-tool-ready This ticket is ready to be merged in label Feb 19, 2024
@inmantaci
Copy link
Contributor

Processing this pull request

inmantaci pushed a commit that referenced this pull request Feb 19, 2024
# Description

add docs/ remove todos
follow-up ticket:
1. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/inmanta-core/7202
2. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/std/504
3. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/apt/281

closes #7134

# Self Check:

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

- [x] 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
- [x] 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 pushed a commit that referenced this pull request Feb 19, 2024
# Description

add docs/ remove todos
follow-up ticket:
1. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/inmanta-core/7202
2. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/std/504
3. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/apt/281

closes #7134

# Self Check:

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

- [x] 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
- [x] 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
Copy link
Contributor

Merged into branches master in 8e189d3

inmantaci pushed a commit that referenced this pull request Feb 19, 2024
# Description

add docs/ remove todos
follow-up ticket:
1. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/inmanta-core/7202
2. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/std/504
3. https://app.zenhub.com/workspaces/core-5a7c152b04a1652024289b7b/issues/gh/inmanta/apt/281

closes #7134

# Self Check:

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

- [x] 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
- [x] 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 Feb 19, 2024
@inmantaci inmantaci deleted the issue/7134-lsm-docs-todos branch February 19, 2024 15:36
@inmantaci
Copy link
Contributor

Processing #7219.

inmantaci pushed a commit that referenced this pull request Feb 19, 2024
Pull request opened by the merge tool on behalf of #7201
@inmantaci
Copy link
Contributor

Processing #7218.

inmantaci pushed a commit that referenced this pull request Feb 19, 2024
Pull request opened by the merge tool on behalf of #7201
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.

LSM documentation landing page contains todos
3 participants