-
Notifications
You must be signed in to change notification settings - Fork 29
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
Phi sat 2 #103
base: dev
Are you sure you want to change the base?
Phi sat 2 #103
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @CesarCoelho - in principle looks good. I found a few things to change.
After the fixups, please ensure to stay compliant with https://github.com/esa/nanosat-mo-framework/blob/dev/CONTRIBUTING.md - most notably use the bug-tracker https://gitlab.com/esa/NMF/nmf-issues for new changes.
...nanosat-mo-connector/src/main/java/esa/mo/nmf/nanosatmoconnector/NanoSatMOConnectorImpl.java
Outdated
Show resolved
Hide resolved
LinuxUsersGroups.printCommandAndOutput(cmd3, out3); | ||
} | ||
|
||
public static void addUserToGroup(String username, String extraGroup) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look like what it is supposed to do. Adding user to a group is typically done via usermod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think usermod was not available on our OBC. We have busybox and I guess this was a work around. Check here: https://stackoverflow.com/questions/37791873/how-to-add-user-to-a-group-without-usermod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linked SO post does not resemble what this method implements. I cannot really match it to either GNU adduser:
https://linux.die.net/man/8/adduser
or busybox adduser:
https://www.busybox.net/downloads/BusyBox.html
core/helper-tools/src/main/java/esa/mo/helpertools/helpers/HelperMisc.java
Outdated
Show resolved
Hide resolved
.../nmf-software-management-impl/src/main/java/esa/mo/sm/impl/provider/AppsLauncherManager.java
Outdated
Show resolved
Hide resolved
@@ -396,6 +407,9 @@ protected boolean isAppRunning(final Long appId) | |||
ret.add(str.toString()); | |||
} else { | |||
if (runAs != null) { | |||
if(sudoAvailable){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If sudo is available, sudo su
is an antipattern as it escapes the sudoers permission lists - i.e. using sudo
+ sudoers configuration is the proper way to ensure that supervisor is not running as a privileged user, and thus commands like
su - user123 -c <command>
should be replaced with sudo -u user123 <command>
. This along with properly tailored system config (i.e. ensuring that user123
belongs to a group which is sudo'able by the supervisor user) will properly utilise linux permissions system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the thorny one... while I agree with you, there was some problem which prevented me from doing what you are suggesting, can't quite recall it however
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please give it a go. Message me if it gives you issues. Perhaps adding -i
switch to sudo is enough in case of problems.
Please also rebase on top of dev branch. |
...nanosat-mo-connector/src/main/java/esa/mo/nmf/nanosatmoconnector/NanoSatMOConnectorImpl.java
Outdated
Show resolved
Hide resolved
.append(withGroup ? " --group " : " ") | ||
.append(username); | ||
} | ||
//String cmd = "useradd $user_nmf_admin -m -s /bin/bash --user-group"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover test code. Remove please.
See the latest round of comments. In addition:
|
e3abc2e
to
d7ac791
Compare
e1dffa8
to
2fbb003
Compare
@@ -307,7 +307,8 @@ public void takePhotograph(long actionInstanceObjId, int stageOffset, int totalS | |||
new Duration(casMCAdapter.getExposureTime()), | |||
casMCAdapter.getGainRed(), | |||
casMCAdapter.getGainGreen(), | |||
casMCAdapter.getGainBlue()); | |||
casMCAdapter.getGainBlue(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API is broken here -> can we patch the StubGenerator to generate multiple constructor wrappers for composites with optional fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I like the idea. Will come with MOv9
552c247
to
476fd81
Compare
1f70921
to
871b4f9
Compare
…radd/userdel to adduser/deluser.
…ollows the AppStorage class recommendations.
…ffecting Phi-Sat-2 setup when starting Apps
…e creation of the storage folders
… default when the transport needs to be generated
… argument object.
…ent folder if it is empty
871b4f9
to
0268bb2
Compare
…g the App Ids directly from the Apps Launcher service instead of getting it from the COM Archive
…ication parameter
The changes needed for the Phi-Sat-2 mission.
This involves mainly (1) the creation of NMF Packages via a maven plugin, (2) support of Apps running in isolation by taking advantage of Linux (one user per App)