[feature] Cria funcionalidade context para trabalhar com multiplos contextos#5
[feature] Cria funcionalidade context para trabalhar com multiplos contextos#5rilder-almeida wants to merge 4 commits intomasterfrom
Conversation
outras features: melhoria dos setup e das variáveis defaults
…ntextos Problema: O kcompose não permitia nativamente trabalhar com multiplos context, tendo que setar a variável KCOMPOSE_CONFIG_FILE manualmente. Solução: Criar a funionalidade context, com as opções set, new, list, delete e show P.s.: As funções setup e login tiveram de ser alteradas, assim como algumas variáveis.
rilder-almeida
requested review from
andremissaglia and
leoeareis
as code owners
|
|
||
| set -f # Disables Globbing | ||
| defaultConfigFile=$HOME/.kcompose/config | ||
| configPath=$HOME/.kcompose/ |
There was a problem hiding this comment.
nit: Essa barra no final fez com que as outras configs ficassem "grudadas", ex: ${configPath}default/. Visualmente acharia mais agradável não colocar a barra aqui e definir como ${configPath}/default
| defaultCredentialsFile=${defaultCredentialsPath}credentials | ||
|
|
||
| if [ ! -f ${configPath}context ]; then | ||
| touch ${configPath}context |
There was a problem hiding this comment.
Acho que não precisa forçar a criação de um arquivo de contexto toda vez. Basta não ler se não estiver criado. Por exemplo, mais abaixo tem:
if [ -f $configFile ]; then
readConfig
fi
|
|
||
| if [ -z "$KCOMPOSE_CONFIG_FILE" ]; then | ||
| if [ ! -d "${configPath}${currentContext}" ]; then | ||
| echo -e "WARNING: Context ${currentContext} not found\n\n" |
There was a problem hiding this comment.
Esse warning vai gerar um desconforto no onboarding do usuário, que vai tomar um warning na primeira vez que executar qualquer comando no kcompose. Fora que o primeiro comando a ser executado deveria ser o kcompose setup, e não o kcompose context set.
Tem uma outra questão que eu não entendi muito bem lendo o código. O kcompose possui uma hierarquia de configurações:
default < arquivo de configuração < variável de ambiente
Como os contextos entram aqui? Me parece que o contexto foi implementado de uma forma conflitante ao KCOMPOSE_CONFIG_FILE. Mas até onde eu entendi, tudo o que muda é o valor de defaultConfigFile: Antes, ele era $HOME/.kcompose/config, e com contextos, ele passa ser $HOME/.kcompose/{context}/config, onde o contexto é "default" se nada estiver setado.
Acho que se for feito dessa forma, conseguimos evitar erros desnecessários ao usuário, e funcionar com pouca configuração
| ;; | ||
| "SASL/SCRAM256") | ||
| ask credentialsFile "Credentials File" $HOME/.kcompose/credentials | ||
| if [ -z "$KCOMPOSE_CONFIG_FILE" ]; then |
There was a problem hiding this comment.
Acho justo deixar de perguntar ao usuário onde salvar o arquivo de credenciais (é uma etapa meio inútil da configuração), mas acho que isso independe se KCOMPOSE_CONFIG_FILE está setado ou não, podemos sempre salvar dentro da pasta do contexto atual.
| setup() { | ||
| ask configFile "Configuration file" $configFile | ||
| ask broker "kafka brokers" $broker | ||
| if [ -z "$1" ]; then |
There was a problem hiding this comment.
Dica para códigos bash: sempre nomeie os parâmetros das funções, porque se não fica difícil saber o que significa $1.
Por exemplo, na função ask, temos:
local result
local question
local default
result=$1
question=$2
default=$3
| ask configFile "Configuration file" $configFile | ||
| ask broker "kafka brokers" $broker | ||
| if [ -z "$1" ]; then | ||
| ask setupConfig "Configuration name" "default" |
There was a problem hiding this comment.
Pelo o que eu entendi, se a pessoa cair nesse fluxo, é porque ela executou kcompose setup. Se cair no fluxo de baixo, é porque ela executou kcompose context new X, e portanto não precisa de um nome, certo?
Então sugiro trocar "default" aqui por $currentContext, assim se a pessoa já estiver em um contexto, ela pode editar sem precisar lembra o nome do contexto em que está.
| } | ||
|
|
||
| # context_show shows the current context | ||
| context_show() { |
There was a problem hiding this comment.
nit: Isso aqui parece ter o mesmo efeito do kcompose env, mas para mostrar o conteúdo de um contexto que não o atual. Será que dá pra extrair uma função de lá, e fazer os dois fluxos chamarem a mesma função? Ou pelo menos manter o output similar, para tentar manter uma experiência consistente para o usuário?
|
|
||
| # validate_name checks if the filename is valid | ||
| validate_name() { | ||
| __name=$1 |
There was a problem hiding this comment.
o bash tem o conceito de variável "local", talvez dê pra usar isso no lugar de __
| # validate_name checks if the filename is valid | ||
| validate_name() { | ||
| __name=$1 | ||
| val=$(echo "${#__name}") |
There was a problem hiding this comment.
Onde isso está sendo usado?
|
|
||
| if [[ $__name == "default" ]]; then | ||
| echo "Context name cannot be 'default'" | ||
| exit |
There was a problem hiding this comment.
importante sair com um código diferente de zero. Isso é um padrão unix para indicar que algo deu errado
2 participants
[feature] Cria funcionalidade context para trabalhar com multiplos contextos
Problema:
O kcompose não permitia nativamente trabalhar com multiplos context, tendo que setar a variável KCOMPOSE_CONFIG_FILE manualmente.
Solução:
Criar a funionalidade context, com as opções set, new, list, delete e show
P.s.: As funções setup e login tiveram de ser alteradas, assim como algumas variáveis.