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

refactor ini file handling logic for write command #454

Merged
merged 1 commit into from
Jun 24, 2024
Merged

refactor ini file handling logic for write command #454

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 48 additions & 27 deletions src/D2L.Bmx/WriteHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using System.Diagnostics;
using System.Text;
using Amazon.Runtime.CredentialManagement;
using IniParser;
using IniParser.Model;

namespace D2L.Bmx;

Expand All @@ -11,6 +13,9 @@ internal class WriteHandler(
BmxConfig config,
FileIniDataParser parser
) {
// this is different from `Encoding.UTF8` which has the undesirable `encoderShouldEmitUTF8Identifier: true`
private static readonly UTF8Encoding Utf8 = new();

public async Task HandleAsync(
string? org,
string? user,
Expand Down Expand Up @@ -64,39 +69,50 @@ bool useCredentialProcess
using( File.Create( output ) ) { };
}

var data = parser.ReadFile( output );
string awsConfigFilePath = useCredentialProcess ? output : SharedCredentialsFile.DefaultConfigFilePath;
var awsConfig = GetParsedIniFileOrNull( awsConfigFilePath );

string awsCredentialsFilePath = useCredentialProcess ? SharedCredentialsFile.DefaultFilePath : output;
var awsCredentials = GetParsedIniFileOrNull( awsCredentialsFilePath );
Comment on lines -67 to +76
Copy link
Member Author

@cfbao cfbao Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the main change.

My main pet peeve in this area is that output and data could have one of two different meanings, depending on the value of useCredentialProcess. I find this confusing lower down where the logic splits into two branches.

So, here, I changed the variable assignment so that output will no longer be used beyond this point, and we will have distinct names for the two different types of files/data.

As a nice side-effect, this is also the last place where we do any INI file parsing, or have any reference to SharedCredentialsFile.


if( useCredentialProcess ) {
string sectionName = $"profile {profile}";
if( !data.Sections.ContainsSection( sectionName ) ) {
data.Sections.AddSection( sectionName );
Debug.Assert( awsConfig is not null );
Copy link
Member

@boarnoah boarnoah Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What is the thought process behind this being an assertion only in debug builds?

EDIT: Also we got BMX_DEBUG, should it just be Assert in all builds that is only tested with BMX_DEBUG = 1 ?

Copy link
Member Author

@cfbao cfbao Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The main goal here is to make the compiler's nullable analysis happy in a clear and intentional way.

awsConfig should not be null here when useCredentialProcess is true (because we'd create the AWS config file above). The compiler cannot infer this, so by default it'd mark awsConfig as "may be null", and issue warnings at L88 and below.

We could use the null-forgiving operator !, but it doesn't make our assumptions and intent clear:

  • Why can we assume awsConfig isn't null in these places?
  • When does this assumption hold?

By using Debug.Assert, we're effectively making clarifying statements like

  • foo isn't null inside this if/else/for/while block
  • bar isn't null once this above statement is executed successfully

We can also avoid sprinkling ! in many different places.

The fact that this is also a runtime assertion in debug builds is more or less a side effect that I don't care that much. I don't think we should make it actually asserts at runtime when BMX_DEBUG is set either.

Copy link
Member Author

Choose a reason for hiding this comment

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

We've used this technique to document our assumptions in code before, e.g.

It's also used by the .NET runtime libraries, e.g.

These are assumptions that should always hold true. If there's a real non-negligible chance that they could be false, then they should become real runtime assertions like

if ( !assumption ) {
	throw new InvalidOperationException( "bad condition" );
}


if( awsCredentials is not null && awsCredentials.Sections.ContainsSection( profile ) ) {
awsCredentials.Sections.RemoveSection( profile );
parser.WriteFile( awsCredentialsFilePath, awsCredentials, Utf8 );
}
if( File.Exists( SharedCredentialsFile.DefaultFilePath ) ) {
var defaultCredentialsFile = parser.ReadFile( SharedCredentialsFile.DefaultFilePath );
if( defaultCredentialsFile.Sections.ContainsSection( profile ) ) {
defaultCredentialsFile.Sections.RemoveSection( profile );
parser.WriteFile( SharedCredentialsFile.DefaultFilePath, defaultCredentialsFile );
}

string sectionName = profile == "default" ? "default" : $"profile {profile}";

if( !awsConfig.Sections.ContainsSection( sectionName ) ) {
awsConfig.Sections.AddSection( sectionName );
}
data[sectionName]["credential_process"] =
awsConfig[sectionName]["credential_process"] =
"bmx print --format json --cache --non-interactive"
+ $" --org {oktaApi.Org}"
+ $" --user {oktaApi.User}"
+ $" --account {awsCredsInfo.Account}"
+ $" --role {awsCredsInfo.Role}"
+ $" --duration {awsCredsInfo.Duration}";

parser.WriteFile( awsConfigFilePath, awsConfig, Utf8 );
} else {
if( File.Exists( SharedCredentialsFile.DefaultConfigFilePath ) ) {
string sectionName = $"profile {profile}";
var defaultConfigFile = parser.ReadFile( SharedCredentialsFile.DefaultConfigFilePath );
if( defaultConfigFile.Sections.ContainsSection( sectionName )
&& defaultConfigFile[sectionName].ContainsKey( "credential_process" ) ) {

if( defaultConfigFile[sectionName].Count == 1 ) {
defaultConfigFile.Sections.RemoveSection( sectionName );
Debug.Assert( awsCredentials is not null );

if( awsConfig is not null ) {
string sectionName = profile == "default" ? "default" : $"profile {profile}";

if(
awsConfig.Sections.ContainsSection( sectionName )
&& awsConfig[sectionName].ContainsKey( "credential_process" )
) {
if( awsConfig[sectionName].Count == 1 ) {
awsConfig.Sections.RemoveSection( sectionName );
} else {
defaultConfigFile[sectionName].RemoveKey( "credential_process" );
awsConfig[sectionName].RemoveKey( "credential_process" );
}
parser.WriteFile( SharedCredentialsFile.DefaultConfigFilePath, defaultConfigFile );
parser.WriteFile( awsConfigFilePath, awsConfig, Utf8 );
Comment on lines -99 to +115
Copy link
Member Author

Choose a reason for hiding this comment

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

nit: without explicitly specifying the encoding, parser will write file using UTF8-BOM 🤢

Console.WriteLine(
"""
An existing profile with the same name using the `credential_process` setting was found in the default config file.
Expand All @@ -106,14 +122,19 @@ bool useCredentialProcess
);
}
}
if( !data.Sections.ContainsSection( profile ) ) {
data.Sections.AddSection( profile );

if( !awsCredentials.Sections.ContainsSection( profile ) ) {
awsCredentials.Sections.AddSection( profile );
}
data[profile]["aws_access_key_id"] = awsCredsInfo.Credentials.AccessKeyId;
data[profile]["aws_secret_access_key"] = awsCredsInfo.Credentials.SecretAccessKey;
data[profile]["aws_session_token"] = awsCredsInfo.Credentials.SessionToken;
awsCredentials[profile]["aws_access_key_id"] = awsCredsInfo.Credentials.AccessKeyId;
awsCredentials[profile]["aws_secret_access_key"] = awsCredsInfo.Credentials.SecretAccessKey;
awsCredentials[profile]["aws_session_token"] = awsCredsInfo.Credentials.SessionToken;

parser.WriteFile( awsCredentialsFilePath, awsCredentials, Utf8 );
}
}

parser.WriteFile( output, data, new UTF8Encoding( encoderShouldEmitUTF8Identifier: false ) );
private IniData? GetParsedIniFileOrNull( string path ) {
return File.Exists( path ) ? parser.ReadFile( path ) : null;
}
}
Loading