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.

Sign up for GitHub

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

Jump to bottom

CFE-3953: added user input support for cfbs #122

Merged
merged 24 commits into from
Oct 10, 2022
Merged

CFE-3953: added user input support for cfbs #122

merged 24 commits into from
Oct 10, 2022

Conversation

larsewi
Copy link
Contributor

@larsewi larsewi commented Jul 12, 2022 •
edited
Loading

Here are some example modules to test with cfbs-test-modules

  • Write documentation
  • Implement cfbs input command
  • Remove input data on cfbs remove
  • Prompt for input on cfbs add
  • Update input on cfbs update
  • Create augments on cfbs build

@cf-bottom
Copy link

Thanks for submitting a pull request! Maybe @olehermanse can review this?

@larsewi larsewi force-pushed the inputs branch 5 times, most recently from 3229acc to c09cc9a Compare July 15, 2022 15:38
vpodzime
vpodzime reviewed Jul 19, 2022
View reviewed changes
Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

Looks promising otherwise. Next is handling inputs on module updates and making sure everything works with dependencies (both when adding modules and updating them).

cfbs/cfbs_config.py Outdated Show resolved Hide resolved
cfbs/cfbs_config.py Outdated Show resolved Hide resolved
@larsewi larsewi force-pushed the inputs branch 6 times, most recently from c34db5b to aa34825 Compare August 5, 2022 12:27
@larsewi larsewi force-pushed the inputs branch 4 times, most recently from ab5f123 to 8c8de21 Compare August 9, 2022 10:39
@larsewi larsewi changed the title CFE-3953: cfbs now prompt for and process module input CFE-3953: added user input support for cfbs Aug 12, 2022
@larsewi larsewi force-pushed the inputs branch 2 times, most recently from f531d24 to 7ee5838 Compare August 12, 2022 14:43
@larsewi larsewi marked this pull request as ready for review August 12, 2022 14:44
@larsewi larsewi requested a review from vpodzime August 12, 2022 14:44
vpodzime
vpodzime reviewed Aug 15, 2022
View reviewed changes
Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

  • instead off` in one of the commit messages

Looks generally OK to me. I don't like the idea of how this whole thing works, but that's another story.

cfbs/result.py Outdated Show resolved Hide resolved
cfbs/commands.py Outdated Show resolved Hide resolved
cfbs/commands.py Outdated Show resolved Hide resolved
cfbs/commands.py Outdated Show resolved Hide resolved
cfbs/commands.py Outdated Show resolved Hide resolved
cfbs/cfbs_config.py Outdated Show resolved Hide resolved
cfbs/cfbs_config.py Outdated Show resolved Hide resolved
cfbs/commands.py Outdated Show resolved Hide resolved
cfbs/commands.py Outdated Show resolved Hide resolved
cfbs/commands.py Show resolved Hide resolved
@larsewi larsewi force-pushed the inputs branch 3 times, most recently from 0d8e607 to 26d3e94 Compare August 15, 2022 14:21
@larsewi larsewi force-pushed the inputs branch 2 times, most recently from 184f485 to 6735c27 Compare September 13, 2022 09:49
olehermanse
olehermanse reviewed Oct 10, 2022
View reviewed changes
cfbs/utils.py Outdated


def canonify(string):
return re.sub(r"[^a-zA-Z0-9_\200-\377]", "_", string)
Copy link
Member

Choose a reason for hiding this comment

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

Should probably reference where this is from (in commit message).

larsewi added 21 commits October 10, 2022 16:26
@larsewi
Added canonify utility function
f24698b 
This function is inspired by the `string_lib.c:CanonifyNameInPlace`
function from libntech:

```C
void CanonifyNameInPlace(char *s)
{
    for (; *s != '\0'; s++)
    {
        if (!isalnum((unsigned char) *s))
        {
            *s = '_';
        }
    }
}
```

I tested it with the CFEngine `canonify` function to see if it produces
the same results. But it responds a little bit differently when using
non-ASCII characters. E.g. the emoji 🐿️ is translated to `___` by
CFEngine but `_` by this python function.

One thing to note is that the `isalnum` function responds differently
based on what locale is set on the system.

Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Add additional files to commit with Result namedtuple
c2c0d3e 
Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
cfbs remove now removes input data
5c356ae 
`cfbs remove` now removes input data. `cfbs remove` now also writes
multiline commit messages as default.

Ticket: CFE-3953
Changelog: Title
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
read_json now handles NotADirectoryError exception
7a8d921 
Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Removed CFBSConfig import from prompts.py
af6b190 
Removed `CFBSConfig` import from prompts.py so that `prompt_user` can be
used in CFBSConfig.py.

Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Improved code in add_command function
c639729 
Changed to use set difference instead off nested for loops.

Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Added command cfbs input
14a801d 
Ticket: CFE-3953
Changelog: Title
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
cfbs add now prompts for input
ae126c4 
`cfbs add` now prompts users whether or not they want to add input for
modules that accept input.

Ticket: CFE-3953
Changelog: Body
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
cfbs update now supports custom index
24b6115 
Ticket: None
Changelog: Title
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Change regex patterns to be raw strings
5da9572 
Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Print message when module is already up to date
689e2b8 
Print message when trying to update a module that is already up to date.

Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Change log message to use '%' string formatting
bb1438c 
My linter complained and said this was better. I don't know why, but I
trust my linter.

Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Fixed bug where updating build step does not commit
66c1c94 
Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
cfbs update now supports modules with input
49042dc 
Ticket: CFE-3953
Changelog: Title
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Added cfbs update (with input) acceptance tests
d0dd0cc 
Ticket: CFE-3953
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Add cfbs update (with input) unit tests
ab34e17 
Ticket: CFE-3953
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Created merge_json unit test
5ff3962 
Created `merge_json` unit test to check my sanity while implementing the
'input' buildstep.

Ticket: None
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Added new 'input' build step for cfbs
5a5c65c 
`cfbs build` now generates augments when it sees the 'input' in build
steps.  If there is no input associated with the 'input' build step, no
operation is done.

Ticket: CFE-3953
Changelog: Title
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Warning when adding a module that already has input
bd40468 
Print a warning when adding a module that already has input data in the
module repository. In the future handle this properly. See ticket
[CFE-4059](https://tracker.mender.io/browse/CFE-4059).

Ticket: CFE-3953
Changelog: Print warning when adding a module that already has input data
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Add shell test for input.
eb66390 
Ticket: CFE-3953
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
@larsewi
Added docs for new cfbs input behaviour
1f6e1fc 
Ticket: CFE-3953
Changelog: None
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
olehermanse
olehermanse reviewed Oct 10, 2022
View reviewed changes
tests/shell/025_add_input_remove.sh
cfbs --non-interactive add example-module
cfbs --non-interactive input example-module
grep '"response": "/tmp/testfile.txt"' ./example-module/input.json
cfbs --non-interactive remove example-module
Copy link
Member

Choose a reason for hiding this comment

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

Should probably check here what remove did.

olehermanse
olehermanse approved these changes Oct 10, 2022
View reviewed changes
Copy link
Member

@olehermanse olehermanse left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for working on this!

@olehermanse olehermanse merged commit 0565700 into cfengine:master Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olehermanse olehermanse olehermanse approved these changes

@vpodzime vpodzime Awaiting requested review from vpodzime

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@larsewi @cf-bottom @vpodzime @olehermanse